MINOR: ConsumerNetworkClient does not need to send the remaining requests when the node is not ready#6264
Merged
guozhangwang merged 1 commit intoapache:trunkfrom Mar 1, 2019
Merged
Conversation
…en the node is not ready
Contributor
Author
|
retest this please |
| if (client.ready(node, now)) { | ||
| client.send(request, now); | ||
| iterator.remove(); | ||
| } else { |
Contributor
There was a problem hiding this comment.
I'm wondering if it is better to do the client.ready() check outside the while loop? If the client is not ready, it will not become ready during the while loop, and vice versa. WDYT @hachikuji ?
Contributor
Author
There was a problem hiding this comment.
Hi @guozhangwang, thank you for your review. IMHO, client.ready() also checks the number of in-flight requests. and the client will become not ready when the number of in-flight requests reaches maxInFlightRequestsPerConnection.
Contributor
There was a problem hiding this comment.
Good point. Makes sense.
Contributor
|
retest this please |
guozhangwang
approved these changes
Mar 1, 2019
Contributor
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
THanks @lambdaliu LGTM.
Member
|
Did we consider adding any tests for this? Since there a change in behaviour, it would be good to have a test. |
Contributor
|
That's good point. @lambdaliu could you file a follow-up PR adding the unit test on ConsumerNetworkClientTest |
Contributor
Author
|
No problem. I'll do it later. |
jarekr
pushed a commit
to confluentinc/kafka
that referenced
this pull request
Apr 18, 2019
* apache/trunk: KAFKA-7880:Naming worker thread by task id (apache#6275) improve some logging statements (apache#6078) KAFKA-7312: Change broker port used in testMinimumRequestTimeouts and testForceClose KAFKA-7997: Use automatic RPC generation in SaslAuthenticate KAFKA-8002; Log dir reassignment stalls if future replica has different segment base offset (apache#6346) KAFKA-3522: Add TimestampedKeyValueStore builder/runtime classes (apache#6152) HOTFIX: add igore import to streams_upgrade_test MINOR: ConsumerNetworkClient does not need to send the remaining requests when the node is not ready (apache#6264) KAFKA-7922: Return authorized operations in describe consumer group responses (KIP-430 Part-1) KAFKA-7918: Inline generic parameters Pt. III: in-memory window store (apache#6328) KAFKA-8012; Ensure partitionStates have not been removed before truncating. (apache#6333) KAFKA-8011: Fix for race condition causing concurrent modification exception (apache#6338) KAFKA-7912: Support concurrent access in InMemoryKeyValueStore (apache#6336) MINOR: Skip quota check when replica is in sync (apache#6344) HOTFIX: Change header back to http instead of https to path license header test (apache#6347) MINOR: fix release.py script (apache#6317) MINOR: Remove types from caching stores (apache#6331) MINOR: Improve logging for alter log dirs (apache#6302) MINOR: state.cleanup.delay.ms default is 600,000 ms (10 minutes). (apache#6345) MINOR: disable Streams system test for broker upgrade/downgrade (apache#6341)
3 tasks
pengxiaolong
pushed a commit
to pengxiaolong/kafka
that referenced
this pull request
Jun 14, 2019
…ests when the node is not ready (apache#6264) Reviewers: Guozhang Wang <wangguoz@gmail.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)