Fix/backfill performance - #63
edge-marge wants to merge 8 commits into
Conversation
orcist
left a comment
orcist
left a comment
There was a problem hiding this comment.
good set of changes, posted a few comments but nothing seriously impacting functionality I think.
| Backfills._Update(newBackfills, $"removed [{backfillID}]"); | ||
| Backfills._Update( | ||
| newBackfills, | ||
| $"{(backfills[backfillID].Status == "ASSIGNED" ? "assigned" : "removed")} [{backfillID}]" |
There was a problem hiding this comment.
Why the check for assigned? We have the polling function which will emit the same update when assigned, this seems duplicate. This is just removing the local reference, removing should always happen only after the ticket has been already assigned or if it has been abandoned. Maybe we can omit the assigned check here?
There was a problem hiding this comment.
it's not duplicate; i moved that update message here & i'm calling RemoveBackfill back again in the polling function (it got removed at some point?) so that the count in AddBackfills is correct => plannedBackfills = TargetTeamSize - (Assignments.Count + Backfills.Current.Count)
There was a problem hiding this comment.
Thank you for explaining -> so we don't emit the removed event for backfills which get assigned. This implies automated removal of assigned backfills, I think it would be better to explicitly emit assigned (in the callback when polling backfills) and then removed right after (in the remove method). Emitting assigned event in the remove method seems counterintuitive and prone to human mistake if we make changes to server agent in the future. Case in point - me misunderstanding the intended flow after reading the code...
Also, some matchmaking options out there do not have this implication and an open backfill feature keeps adding players until manually stopped. I'd rather make it obvious by reading the log that it was assigned and removed right after.
| using System.Collections; | ||
| using System.Collections.Concurrent; | ||
| using System.Collections.Generic; | ||
| using System.Linq; |
There was a problem hiding this comment.
is this important really needed? maybe accidentally included?
There was a problem hiding this comment.
yeah its's needed for when I check whether the consecutiveErrors counter needs to be incremented or reset when polling
There was a problem hiding this comment.
Are you sure about that? Maybe I'm misunderstanding, but that counter is just an integer, that shouldn't need linq.
EDIT: I think I get it, you need linq for the ConcurrentQueue.Contains method?
orcist
left a comment
orcist
left a comment
There was a problem hiding this comment.
small change requests, feel free to DM me if it helps close this faster.
2 participants
No description provided.