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
overload: scale transport socket connect timeout #13800
overload: scale transport socket connect timeout #13800
Changes from 11 commits
abcdb84
f5b8d56
7d28234
5d0d600
76cd8c4
2f169bf
0c1cf92
3376062
927ea02
68e62d4
9d85d8a
29ea543
f52c76e
a5ccd82
18f6022
4ca4b11
ee93040
ddad3e5
9be6e8f
475253b
2f2ed9f
6ad33cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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'm worried that adding this and the related inline methods here may have a detrimental effect on compile-time. Does this need to live in a header?
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 method definitions could be moved to a .cc file. Should I put that under
include
or move the whole thing tosource/common/common
?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.
These ostream helpers don't have to be defined inside the struct and you don't even need
friend
since the members are public, so we can define in another .h files, where are they used?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.
They're used in ScaledTimerMinimum::operator<<, which does depend on private state.
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 did a local change that undid changes to this line and fixed all the unused variable compile errors, and noticed that the only test that failed //test/common/network:connection_impl_test due to changes to mock expectations. It would be good to have some e2e integration test coverage for this functionality.
This issue also came up in #14155 . We have multiple scaled timers, we should have some generic recipes for how to integration test scaled timer functionality. Possible strawman drive the connection to a certain state, force proxy into overload and verify that the timeout triggers shortly afterwards.
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.
Sent #14290 to do just that for the existing scaled timer.
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 wonder if we should revisit the possibility of having the dispatcher be the one that is aware of the overload state and provide a method to create scaled timers. Dispatcher is plumbed in widely.
I can see a counter argument involving our desire to bring overload manager closer to connections as a starting point for adjusting work done on wakeup based on overload state; something like smaller allocations/reads when under memory pressure. So this new plumbing may be more generally useful. But I think the direction we're taking there is to measure memory usage by connection, which actually doesn't require involvement from the overload mananger.
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.
As mentioned elsewhere, I'd rather keep scaled timer creation going through the Overload Manager since it is ultimately responsible for determining the scale factor of those timers. We can revisit making the Dispatcher aware of the OM later.
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.
Instead of plumbing the OM widely, could the dispatcher hold a reference to it? I dislike that there are now two very different places to create timers: OM and dispatcher.
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.
There's a chicken-and-egg problem there since the thread-local overload state requires a reference to the dispatcher. We could work around this, though, if that sounds preferable. It seems like the larger issue is that there is a divide between the dispatcher, which represents a thread worker, and thread-local state, which is managed elsewhere.
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 dispatcher is also owned by the thread. I wonder if the solution would be to have the dispatcher own the thread overload state instead of having it be part of the thread local storage.
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.
Yeah I think that makes sense.
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.
+1
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 started working on this. It's doable but introduces some weirdness; see #14401 for more.
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.
Made a fresh attempt at this in #14679 which feels reasonably clean.