Skip to content

KAFKA-9530; Fix flaky test testDescribeGroupWithShortInitializationTimeout #8154

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.

Sign up for GitHub

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

Merged
hachikuji merged 3 commits into from
Feb 23, 2020
Merged

KAFKA-9530; Fix flaky test testDescribeGroupWithShortInitializationTimeout #8154

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d70807
KAFKA-9530; Fix flaky test `testDescribeGroupWithShortInitializationT…
hachikuji Feb 21, 2020
a1db9a0
retryable -> retriable
hachikuji Feb 22, 2020
631112b
Use TimeoutException when retries are exhausted
hachikuji Feb 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 15 additions & 18 deletions clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,11 +710,7 @@ final void fail(long now, Throwable throwable) {
// TimeoutException. In this case, we do not get any more retries - the call has
// failed. We increment tries anyway in order to display an accurate log message.
tries++;
if (log.isDebugEnabled()) {
log.debug("{} aborted at {} after {} attempt(s)", this, now, tries,
new Exception(prettyPrintException(throwable)));
}
handleFailure(new TimeoutException("Aborted due to timeout."));
failWithTimeout(now, throwable);
return;
}
// If this is an UnsupportedVersionException that we can retry, do so. Note that a
Expand All @@ -731,14 +727,10 @@ final void fail(long now, Throwable throwable) {

// If the call has timed out, fail.
if (calcTimeoutMsRemainingAsInt(now, deadlineMs) < 0) {
if (log.isDebugEnabled()) {
log.debug("{} timed out at {} after {} attempt(s)", this, now, tries,
new Exception(prettyPrintException(throwable)));
}
handleFailure(throwable);
failWithTimeout(now, throwable);

@ijuma ijuma Feb 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Below, we have a case where we throw the underlying exception if we run out of retries. Is that intentional?

@hachikuji hachikuji Feb 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was debating changing that one too. If retries are exhausted, do we consider that a timeout? I guess it's defensible.

@ijuma ijuma Feb 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, if retries are exhausted and it's a retriable exception, then it seems like it should be a TimeoutException.

return;
}
// If the exception is not retryable, fail.
// If the exception is not retriable, fail.
if (!(throwable instanceof RetriableException)) {
if (log.isDebugEnabled()) {
log.debug("{} failed with non-retriable exception after {} attempt(s)", this, tries,
Expand All @@ -749,11 +741,7 @@ final void fail(long now, Throwable throwable) {
}
// If we are out of retries, fail.
if (tries > maxRetries) {
if (log.isDebugEnabled()) {
log.debug("{} failed after {} attempt(s)", this, tries,
new Exception(prettyPrintException(throwable)));
}
handleFailure(throwable);
failWithTimeout(now, throwable);
return;
}
if (log.isDebugEnabled()) {
Expand All @@ -763,6 +751,15 @@ final void fail(long now, Throwable throwable) {
runnable.enqueue(this, now);
}

private void failWithTimeout(long now, Throwable cause) {
if (log.isDebugEnabled()) {
log.debug("{} timed out at {} after {} attempt(s)", this, now, tries,
new Exception(prettyPrintException(cause)));
}
handleFailure(new TimeoutException(this + " timed out at " + now
+ " after " + tries + " attempt(s)", cause));
}

/**
* Create an AbstractRequest.Builder for this Call.
*
Expand All @@ -783,7 +780,7 @@ final void fail(long now, Throwable throwable) {

/**
* Handle a failure. This will only be called if the failure exception was not
* retryable, or if we hit a timeout.
* retriable, or if we hit a timeout.
*
* @param throwable The exception.
*/
Expand Down Expand Up @@ -1115,7 +1112,7 @@ private void handleResponses(long now, List<ClientResponse> responses) {
}

// Handle the result of the call. This may involve retrying the call, if we got a
// retryible exception.
// retriable exception.
if (response.versionMismatch() != null) {
call.fail(now, response.versionMismatch());
} else if (response.wasDisconnected()) {
Expand Down