-
Notifications
You must be signed in to change notification settings - Fork 14.6k
MINOR: Deflake streams admin api describe streams group with short timeout #20320
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
base: trunk
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR @aliehsaeedii - overall LGTM - I'll kick off the build a few times to see if the flakiness is fixed, and either merge or discuss if we need to keep the test.
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.
What was the original purpose of the test? -- It's unclear to me, why creating a second KS instance help to stabilize the test. Can you elaborate?
If I read the test correctly, the idea is to get a timeout exception for a describe
call -- what is the conditions when we would get a timeout exception? Group does not exist? Or something else?
public void testDescribeStreamsGroupWithShortTimeout() { | ||
public void testDescribeStreamsGroupWithShortTimeout() throws Exception { | ||
cluster.createTopic(INPUT_TOPIC_2, 1, 1); | ||
KafkaStreams streams2 = new KafkaStreams(topology(INPUT_TOPIC_2, OUTPUT_TOPIC_2), streamsProp(APP_ID_2)); |
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.
We should use try-with-resource.
List<String> args = List.of("--bootstrap-server", bootstrapServers, "--describe", "--members", "--verbose", "--group", APP_ID, "--timeout", "1"); | ||
Throwable e = assertThrows(ExecutionException.class, () -> getStreamsGroupService(args.toArray(new String[0])).describeGroups()); | ||
assertEquals(TimeoutException.class, e.getCause().getClass()); | ||
|
||
streams2.close(); | ||
streams2.cleanUp(); |
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.
If we use try-with-resource, we can remove close()
and put cleanUp()
into finally-block.
With a
1ms
timeout, the streams group description occasionallysucceeds. To check if this resolves the flakiness, I created the streams
group within the test itself. If this approach doesn't stabilize the
test, we should consider removing it altogether, since the original
intent behind the test may no longer be valid.
Reviewers: Bill Bejeck[email protected]