Skip to content
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

KAFKA-7939: Fix timing issue in KafkaAdminClientTest.testCreateTopicsRetryBackoff #6418

Closed
wants to merge 2 commits into from

Conversation

omkreddy
Copy link
Contributor

@omkreddy omkreddy commented Mar 10, 2019

There is a small timing window where time.sleep(retryBackoff) will get executed before adminClient adds retry request to the queue. Added a check to wait until the retry call added to the queue in AdminClient.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@omkreddy
Copy link
Contributor Author

There is a small timing window where time.sleep(retryBackoff) will get executed before adminClient adds retry request to the queue. Due to this, retry request's nextAllowedTryMs will get updated with new time. then maybeDrainPendingCalls method waits continuously here

Copy link
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omkreddy Thanks for the PR. Left one minor comment, apart from that LGTM.

@@ -1261,6 +1261,11 @@ private static boolean groupIdIsUnrepresentable(String groupId) {
return groupId == null;
}

//for testing
int noOfPendingCalls() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps numPendingCalls is easier to read?

@omkreddy
Copy link
Contributor Author

@rajinisivaram Thanks for the review.

omkreddy added a commit that referenced this pull request Mar 11, 2019
…RetryBackoff

There is a small timing window where ```time.sleep(retryBackoff)``` will get executed before adminClient adds retry request to the queue.  Added a check to wait until the retry call added to the queue in AdminClient.

Author: Manikumar Reddy <manikumar.reddy@gmail.com>

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>

Closes #6418 from omkreddy/KAFKA-7939

(cherry picked from commit 173a7e3)
Signed-off-by: Manikumar Reddy <manikumar@confluent.io>
@omkreddy omkreddy closed this in 173a7e3 Mar 11, 2019
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…RetryBackoff

There is a small timing window where ```time.sleep(retryBackoff)``` will get executed before adminClient adds retry request to the queue.  Added a check to wait until the retry call added to the queue in AdminClient.

Author: Manikumar Reddy <manikumar.reddy@gmail.com>

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>

Closes apache#6418 from omkreddy/KAFKA-7939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants