Roll-up: Flow control changes, and internal timing changes#12677
Merged
ctiller merged 115 commits intogrpc:masterfrom Oct 6, 2017
Merged
Roll-up: Flow control changes, and internal timing changes#12677ctiller merged 115 commits intogrpc:masterfrom
ctiller merged 115 commits intogrpc:masterfrom
Conversation
|
|
|
1 similar comment
|
|
|
|
Member
|
I assume that Noah has taken a close look at the chttp2 code and the BDP estimator code, so I didn't look at any of that. If you want me to, please let me know. For the rest, all comments are minor. Reviewed 167 of 167 files at r1. src/core/lib/channel/handshaker.h, line 152 at r1 (raw file):
As I noted in the original PR, this will require some internal changes when we import it. Please have those changes ready before merging this PR. src/core/lib/iomgr/ev_epollex_linux.cc, line 61 at r1 (raw file):
Nit: Can we put src/core/lib/iomgr/ev_epollex_linux.cc, line 692 at r1 (raw file):
Seems like this same function exists in a number of pollers. Maybe pull it out somewhere else so the code can be shared? src/core/lib/surface/call.cc, line 1345 at r1 (raw file):
This looks unrelated to the rest of the PR. I assume this was a bug you discovered during testing? test/core/end2end/invalid_call_argument_test.c, line 95 at r1 (raw file):
Why is this change needed? test/core/end2end/fixtures/h2_ssl_cert.c, line 322 at r1 (raw file):
Same question here. test/core/transport/status_conversion_test.c, line 31 at r1 (raw file):
What's the purpose of wrapping this in a do loop that always terminates? If it's just to declare the exec_ctx in its own scope, couldn't we just create a bare scope here, without the loop? test/cpp/common/alarm_cpp_test.cc, line 145 at r1 (raw file):
Why is this change needed? Can we no longer use a timeout of 0? test/cpp/common/alarm_cpp_test.cc, line 161 at r1 (raw file):
Same question here. test/cpp/end2end/generic_end2end_test.cc, line 148 at r1 (raw file):
What's the reason for this change? Comments from Reviewable |
markdroth
approved these changes
Oct 6, 2017
Member
Author
|
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed. src/core/lib/channel/handshaker.h, line 152 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
I'll do the import for this merge. src/core/lib/iomgr/ev_epollex_linux.cc, line 61 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/lib/iomgr/ev_epollex_linux.cc, line 692 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Ack. There'll be a code cleanup round soon... I'd like to keep as much as possible separate until things settle here however. src/core/lib/surface/call.cc, line 1345 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Yeah, if we receive a full message we return it and clients leak it right now. test/core/end2end/invalid_call_argument_test.c, line 95 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Nothing else guarantees the connection will be ready, causing this test to be flaky (and more so with the timer changes) test/core/end2end/fixtures/h2_ssl_cert.c, line 322 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Same answer test/core/transport/status_conversion_test.c, line 31 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
A bare scope leads to one of:
Wrapping in a do{}while() allows the semicolon to be valid and the macro to 'work as expected' - it's a fairly common C macro idiom test/cpp/common/alarm_cpp_test.cc, line 145 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
We can, but it's of insufficient length to actually catch the timeout... it was working by coincidence previously. test/cpp/common/alarm_cpp_test.cc, line 161 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Same answer. test/cpp/end2end/generic_end2end_test.cc, line 148 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
The rounding changed slightly, and for some of the longer deadlines we're passing, 100ms no longer bounds the error. This is too large, but I think good enough for the purposes here. Comments from Reviewable |
|
|
|
|
|
|
This was referenced Oct 6, 2017
Closed
This was referenced Oct 10, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
10 participants
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.
Combines #11072 and #11866 to be merged together (and maintained together until merging)
This change is