Allocation: Re-cache allocated GameServer#4159
Conversation
|
Build Succeeded 🥳 Build Id: 800070be-37f3-4e69-b24f-15f108a1ac20 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: |
GameSererGameServer
|
@dmorgan-blizz FYI this is one proposal for your issue #3992. Let us know how this performs. |
There should be no downside to this change, it should help no matter what. |
|
Any blocking reason this shouldn't be merged? |
|
Build Succeeded 🥳 Build Id: 2c277f71-ca90-4eb7-9ef5-a04a815ff008 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: |
peterzhongyi
left a comment
There was a problem hiding this comment.
Should we worry about the informer giving a stale update on the game server that overwrites the game server record you write here? Seems like it might happen frequently given that the informer is almost always going to be slower but will still try to write the the cache?
|
🤔 I can't think of an example, since the Informer is only going to give you the latest version of the We could do a generation check on store, and if it's less than the latest version that is stored in there (which is possibly not a bad idea anyway), we reject it. Actually, that's a pretty good idea anyway, I'll put that in 👍🏻 seems like thing that shouldn't happen often (ever?), but easy enough to make sure it doesn't. |
We want the allocated `GameServer` back in the cache anyway for re-allocation, so might as well get it back in as soon as possible. Work on agones-dev#3992
|
Sounds good. LGTM! |
|
Build Succeeded 🥳 Build Id: 9a44c0be-ec98-48a9-9c04-0b3be35b39e6 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: |
c1199d6 to
9eed0e6
Compare
| if existingGS, ok := e.cache[key]; ok { | ||
| // If the incoming gs object's Generation is less than the existing one, ignore it, but also check the ResourceVersion | ||
| // saves us a deep copy if we don't need to | ||
| if gs.ObjectMeta.Generation < existingGS.ObjectMeta.Generation || gs.ObjectMeta.ResourceVersion == existingGS.ObjectMeta.ResourceVersion { |
There was a problem hiding this comment.
I had forgotten that you can get resourceVersion change, but with no Generation change, so you have to account for it, and lean on eventual consistency when it's needed.
|
Build Failed 😭 Build Id: 7aebfd4a-d18d-4d04-9bd5-ebe1ec51fb2e Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: f4d08f7b-4d6a-4cfd-bb5c-144be473a4a0 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: |
What type of PR is this?
/kind bug
What this PR does / Why we need it:
We want the allocated
GameServerback in the cache anyway for re-allocation, so might as well get it back in as soon as possible.Which issue(s) this PR fixes:
Work on #3992
Special notes for your reviewer: