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-17990: Flaky test improvements #17814

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

AndrewJSchofield
Copy link
Collaborator

Several of the ShareConsumerTest integration tests are a bit flaky. This PR tightens up the logic with the aim of eliminating the flakes. Annoyingly the tests seem rock solid locally so this might take some experimentation.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added core Kafka Broker tests Test fixes (including flaky tests) small Small PRs labels Nov 14, 2024
@AndrewJSchofield
Copy link
Collaborator Author

ShareConsumerTest.testShareAutoOffsetResetDefaultValue depended upon setting the fetch position after a record was appended to the partition. There was the possibility that these happened in the wrong order and a record was received unexpectedly.

Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a 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, seems good to me. I will wait for the test and might try to run couple of times to see the flakiness behaviour.

@@ -322,6 +325,7 @@ public void testAcknowledgementCommitCallbackSuccessfulAcknowledgement(String pe
ProducerRecord<byte[], byte[]> record = new ProducerRecord<>(tp.topic(), tp.partition(), null, "key".getBytes(), "value".getBytes());
KafkaProducer<byte[], byte[]> producer = createProducer(new ByteArraySerializer(), new ByteArraySerializer());
Copy link
Contributor

Choose a reason for hiding this comment

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

should the closeable resources be closed in a try / finally block so that the resources are closed even if we have an assertion failure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that's probably a good thing to do. I'll put in the next commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, with the exception of the tests which use ExecutorService. I'll see if I can improve those too.

@github-actions github-actions bot removed the small Small PRs label Nov 14, 2024
@AndrewJSchofield
Copy link
Collaborator Author

We recently changed the default starting position for consumption from earliest to latest to match the KIP, which necessitated alter configs for the tests to "earliest". The point at which this takes effect is non-deterministic and I believe that's responsible for the increased flakiness.

shareConsumer.close();
producer.close();
try (KafkaProducer<byte[], byte[]> producer = createProducer(new ByteArraySerializer(), new ByteArraySerializer());
KafkaShareConsumer<byte[], byte[]> shareConsumer = createShareConsumer(new ByteArrayDeserializer(), new ByteArrayDeserializer(), "group1")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to close shareConsumer here and other places ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happens automatically because these resources are opened in a try-with-resource block. As the scope of the try block is exited, the resources are automatically closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker KIP-932 Queues for Kafka tests Test fixes (including flaky tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants