googleforgames / open-match

Flexible, extensible, and scalable video game matchmaking.
http://open-match.dev
Apache License 2.0
3.18k stars 335 forks source link

API change: Move assignment off of ticket #1139

Closed Laremere closed 4 years ago

Laremere commented 4 years ago

Overview

Remove assignment field from ticket. Add assignment field to GetTicketResponse.

Motivation

Assignment is the only mutable field on ticket, and isn't populated in most places. The only place it is populated is on a get ticket call. I think we should move assignment to another field on the response from get ticket.

Impact

The unusable assignment field is no longer visible from many places (mmf, evaluator, etc.)

https://github.com/googleforgames/open-match/issues/1033 moves from being an error to impossible.

If a user wants to pass in an assingment, eg for backfill, it can be done on an extension.

Change Proto

Ticket becomes:

message Ticket {
  // Id represents an auto-generated Id issued by Open Match.
  string id = 1;

  // Search fields are the fields which Open Match is aware of, and can be used
  // when specifying filters.
  SearchFields search_fields = 4;

  // Customized information not inspected by Open Match, to be used by the match
  // making function, evaluator, and components making calls to Open Match.
  // Optional, depending on the requirements of the connected systems.
  map<string, google.protobuf.Any> extensions = 5;

  // Deprecated fields.
  reserved 2, 3;
}

Update to FrontendService.GetTicket (it currently just returns a ticket object directly, should be changed anyways.)

  // GetTicket get the Ticket associated with the specified TicketId.
  rpc GetTicket(GetTicketRequest) returns (GetTicketResponse) {
    option (google.api.http) = {
      get: "/v1/frontendservice/tickets/{ticket_id}"
    };
  }

message GetTicketResponse {
  // The ticket with the requested id.
  Ticket ticket = 1;

  // The assignment associated with the ticket.  Only present if the
  // ticket has been assigned.
  Assignment assignment = 3;
}
sawagh commented 4 years ago

Conceptually Assignment to Ticket is a 1:1 mapping today. If we do plan to make that 1-n out of this change and if there is a strong reason to do that, I see the need for this change. However, that would also complicate the implementation and so should not be done unless there is a need to. Given every Ticket is associated with an Assignment and an Assignment without a ticket does not make sense, having them together is conceptually simple.

I am unclear about the motivation here. Being the only mutable field - and not mutated in most places - does not seem a strong motivation for a breaking change at this stage. (Is there a customer motivation, is there a perf implication?).

Even the impact is not significant to justify the change. If anything, for backfill, using extensions for an assignment seems more like a workaround (than use the assignment field that exists today) so i almost think of that as a deterrent to this change.

sawagh commented 4 years ago

Discussed this in TSC and there was agreement on there not being a strong motivation to make this change. The recommendation is to not address this in v1.0 timeframe - unless we are able to identify significant perf savings by doing this.

Laremere commented 4 years ago

TSC re-review (with me present) agreed to not go forward with this.