Refactor gameserverallocations to its components #1015
Conversation
|
Build Failed 😱 Build Id: 6057f975-d3ee-4c27-b831-96ff21bd081c To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
pooneh-m
force-pushed
the
dev
branch
2 times, most recently
from
c58945d
to
521f030
Compare
|
Build Failed 😱 Build Id: 4121a51b-3362-4b6d-9b67-15ef9f39d592 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 8ecb1c89-26aa-4dce-9c5b-9028aceb16ce To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
I am investigating the failures, but please give an early feedback on the refactoring if it makes sense. |
|
I'll take a look shortly. |
|
Build Failed 😱 Build Id: ce35fa63-351d-4b83-81ce-54bbd78326ad To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 👏 Build Id: be10af58-b2ed-471c-bc2e-a63d4555dc55 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
| } | ||
|
|
||
| // Enqueue enqueues a game server to be synced. Using queue helps avoiding multiple threads syncing at the same time. | ||
| func (c *CacheHandler) Enqueue(gs *agonesv1.GameServer) { |
There was a problem hiding this comment.
Should we take the opportunity to clean this up a little?
Make this something like EnqueueResync() or similar, that takes no arguments. The code never actually checks the key, so we could pass in an arbitrary string.
Might make more sense semantically. WDYT?
There was a problem hiding this comment.
Hmm why are we syncing the whole cache and not just the one that is out of sync?
There was a problem hiding this comment.
@ilkercelikyilmaz can you provide more context? I believe you wrote this part.
From memory, I think this is used only in a complete "we're no longer sure what's happening, so lets sync everything just in case", such as here
| ) | ||
|
|
||
| // CacheHandler handles the gameserver sync operations for cache | ||
| type CacheHandler struct { |
There was a problem hiding this comment.
Should be call this a ReadyGameServerCache or something similar?
(and maybe the file should be cache.go?)
There was a problem hiding this comment.
How about ready_gs_cache.go?
There was a problem hiding this comment.
That works - I tend to like shorter file names, and since file names in go aren't 1:1 to object definitions (like java), I usually go for something a little less specific if I don't have to.
But I'm not wedded to it - ready_gs_cache.go is fine.
There was a problem hiding this comment.
or compromise: ready_cache.go ? 😄
|
Thanks @markmandel for the review. The naming suggestions are great. |
|
Build Succeeded 👏 Build Id: 98807806-16e4-47be-92d0-138ff13bc729 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: markmandel, pooneh-m The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
Build Succeeded 👏 Build Id: 1c452e71-a60b-4150-b8fd-0498ed40e31d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
gameserverallocations is a module with multiple independent responsibilities.
This refactoring does not change any logic and only moves the code around to its rightful modules to simplify and also to make the allocation library reusable by agones-allocator service to address #1018 with parent issue #597.