Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
server: make overload manager non-optional #11777
server: make overload manager non-optional #11777
Changes from all commits
09a4d7c
76f494b
0a402c4
2609dbe
dac81f8
159f964
5f40260
44ac271
7334861
438f711
ed7e988
3bc1f88
04053d4
82146f6
c190617
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is start() called?
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.
Nope, moved the TLS initialization to the constructor, which seems to work fine.
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 think you opted to use start() in the end.
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.
What was the reasoning to keep this in start vs the ctor?
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.
The constructor gets invoked with AdminImpl's constructor, which is invoked in server.cc before the TLS state is initialized. Putting this in start lets us defer it.
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.
const ThreadLocalOverloadState& since ThreadLocalOverloadState has no mutation methods as part of the interface.
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 don't want to make this const because I expect in the near future that we'll want non-const methods on here.
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.
Tell me more about said non-const methods. I would expect OverloadManager to be the one doing all the updates to overload state resources. I'm guessing that you're referring to methods to create scalable timers, which in fact would need to be non-const.
That said, I wonder if scalable timers should be more closely integrated with the Event::Dispatcher interface.
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.
Yep, my concern is around adding methods to create scalable timers. I think we can discuss them in a separate issue, though, and leave this as-is here.
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 think my advice is to use the Event::Dispatcher interface to create scalable timers. We can discuss in the PR that defines that API.
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 think I'd prefer to make this const for now and then remove it down the line if we start needing non-const access, just so we don't accidentally leave this non-const.
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.
Finally gave it a try and it turns out that won't work since getState() is non-const. getState is non-const because it looks up a key in the internal state map and creates an entry for it if one doesn't exist.
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.
It is strange that getState returns a reference instead of a value. State is a primitive type (an enum)
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.
The callers hold onto the reference to avoid extra map lookups
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.
What specific action you need to happen first? Is it the "thread_local_.registerThread(*dispatcher_, true);"
Does worker initialization happen before or after this point?
I wonder if initialization is still happening too early and possible consequences for internal admin API users which bypass request handling on the admin port.
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.
Yep, that's the line. The worker initialization happens before, in the ListenerManagerImpl constructor.