-
Notifications
You must be signed in to change notification settings - Fork 817
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
Speed up creation/deletion of game servers in a set #542
Speed up creation/deletion of game servers in a set #542
Conversation
Build Failed 😱 Build Id: 1b95cdde-44fb-447d-9605-251262bd7128 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
e3c6ab4
to
54c077c
Compare
Build Failed 😱 Build Id: 26be4f54-3be0-46fe-82a8-0d0c7a234151 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: c46f8ace-8882-482e-9a5a-9ab9da04b119 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: e484653c-5ac5-4572-ba50-9e08152341fb To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 67b09fe2-2b8d-4f56-99b7-0cd54e2f27d7 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will wait to do a proper review once you're ready - but was just reading through, found a few minor cleanup things.
86e6868
to
19f98b5
Compare
Build Succeeded 👏 Build Id: d523e7e5-41c5-4c6a-adc8-4436300b3934 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
Build Succeeded 👏 Build Id: 3ce57273-0b61-490d-b2e1-085cbedd55fd The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably go through this again, I want to make sure I understand what's happening here.
Added some notes though for mostly minor cleanup things.
19f98b5
to
d173b4a
Compare
Build Succeeded 👏 Build Id: cc8bc9f1-d5ab-40be-b56b-4817ef843b41 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
pkg/gameservers/controller.go
Outdated
c.workerqueue = workerqueue.NewWorkerQueue(c.syncGameServer, c.logger, stable.GroupName+".GameServerController") | ||
c.workerqueue = workerqueue.NewWorkerQueueWithRateLimiter(c.syncGameServer, c.logger, stable.GroupName+".GameServerController", | ||
workqueue.NewItemFastSlowRateLimiter(20*time.Millisecond, 500*time.Millisecond, 5)) | ||
c.creationWorkerQueue = workerqueue.NewWorkerQueueWithRateLimiter(c.syncGameServer, c.logger, stable.GroupName+".GameServerControllerCreation", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider having separate method for each workerqueue instead of syncGameServer method? Each method can only focus on the possible events rather than trying to process all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queue is just a "dirty" bit, it's totally possible that the state of a resource will change underneath between enqueue and processing, so we should not assume anything about the state of each GS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your concern about queue. However the new "enqueueGameServerBasedOnState" method should put the messages to the appropriate queue. I think except syncGameServerDeletionTimestamp handler, every other handler ignore unexpected events.
d173b4a
to
a9d1ba4
Compare
Build Succeeded 👏 Build Id: 9aa1bfe8-8b91-4048-a754-aea47973403b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
caf5cda
to
32cab47
Compare
Build Succeeded 👏 Build Id: bc1da740-7d27-4cd9-ba8b-1ce95f647413 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
Build Succeeded 👏 Build Id: 3ffcf1db-2549-4fef-8d70-cb5e066c24e0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more questions/comments, but otherwise I think this is good to go. Just took me a while to run through it, and understand it completely.
pkg/gameserversets/controller.go
Outdated
|
||
// GS being deleted counts towards target replica count. | ||
if !gs.ObjectMeta.DeletionTimestamp.IsZero() { | ||
//handleGameServerUp(gs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
32cab47
to
2c821ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw the health check, and a blank line cleanup - otherwise, good to go.
- parallelizing create/delete calls in GSS controller - creating dedicated worker queues for creation/deletion requests - replacing default rate limiters with ones that don't have global QPS limit
2c821ed
to
b8d0644
Compare
Build Succeeded 👏 Build Id: fced92f7-08c5-41ec-b203-eb4ee3e9c60b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Build Succeeded 👏 Build Id: 8561d6f5-a946-41e8-ba21-291f52fcd4b7 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website will exist for the next 30 days: To install this version:
|
That, plus general cleanup of GSS controller for readability.
Also, introduced GS state cache, so that actions taken by GSS controller are more precise because they don't rely on stale informer data.