Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API change: Have evaluator return only match ids #939

Closed
Laremere opened this issue Nov 4, 2019 · 2 comments · Fixed by #1078 or #1082
Closed

API change: Have evaluator return only match ids #939

Laremere opened this issue Nov 4, 2019 · 2 comments · Fixed by #1078 or #1082
Assignees
Labels
breaking api change Breaking API change that may require migration for existing customers to update to new version.
Milestone

Comments

@Laremere
Copy link

Laremere commented Nov 4, 2019

Overview

Instead of the evaluator returning a stream of matches, only return the ids of the matches it wants to form.

Motivation and Impact

Currently, the evaluator returns the entire protobuf of the matches it wishes to form. This means that the entire protobuf is re-serialized and sent back to the caller which sent the protobuf in the first place. (The same thing happens in the backend to synchronizer call, but that's internal, so can be changed without an externally visible breaking change.) It would be much more efficient to only return the id.

One downside is that the evaluator can't change the contents of any matches (eg, dropping a player seen in another match, but still making the match as a whole.) It's not clear that it's supported anyway (I don't know what the code does today.) Generally, I think it's better if things are treated as immutable.

Change Proto

From:

message EvaluateResponse {
  // A Match shortlisted by the evaluator representing one of the final results.
  Match match = 1;
}

To:

message EvaluateResponse {
  // A Match shortlisted by the evaluator representing one of the final results.
  string match_id = 1;
}
@Laremere Laremere added the breaking api change Breaking API change that may require migration for existing customers to update to new version. label Nov 4, 2019
@Laremere Laremere added this to the v0.9.0 milestone Nov 4, 2019
@calebatwd
Copy link
Collaborator

Sticking to just returning matchids should be fine! If we need to make a future extension point, we can justify it then :)

@Laremere Laremere assigned yfei1 and Laremere and unassigned calebatwd and yfei1 Jan 16, 2020
@Laremere
Copy link
Author

Execution plan

As seperate PRs for each of the steps, do the following:

  1. First handle the changes for calling the mmf from the backend for Start streaming where possible in more places. #940 This will touch the same code, so it makes sense to rework first.

  2. Add a sync.map to the backend fetch matches call. When a match is returned from the MMF, add it to the map before sending it to the synchronizer. When a match is returned from the synchronizer, take its id and use the value from the map. This means we'll ignore changes on the match made by the evaluator during this intermediate step, but the evaluator will still return the full match.

  3. Change the interal API for the synchronizer to only return the id from the match, instead of the full match. Basically the match should be returned from the evaluator and immediately turned into only the id.

  4. Change the external evaluator API to only return the id.

The goal of breaking up the changes here is to keep them small. 4 will involve changes to the callers of the API, so keeping the change to the internal syncrhonizer small as possible makes the change easier to review.

yfei1 added a commit that referenced this issue Jan 30, 2020
This is an intermediate step to resolve #939. Leaving a bunch of TODOs in this PR and will fix them after the proto change.
@Laremere Laremere reopened this Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking api change Breaking API change that may require migration for existing customers to update to new version.
Projects
None yet
3 participants