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.

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

Jump to bottom

KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test #13664

Merged
merged 1 commit into from May 26, 2023
Merged

KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDown test #13664

merged 1 commit into from May 26, 2023

Conversation

machi1990
Copy link
Contributor

  1. Ensures that NPE are not thrown
  2. Ensures that the background thread has been started to avoid flasky assertions failures on isRunning
  3. Add a check that the thread is not running when closed

Committer Checklist (excluded from commit message)

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

@machi1990
KAFKA-14961: harden DefaultBackgroundThreadTest.testStartupAndTearDow…
6c3e82f 
…n test

1. Ensures that NPE are not thrown
2. Ensures that the background thread has been started to avoid flasky
   assertions failures on isRunning
3. Add a check that the thread is not running when closed
machi1990
machi1990 commented May 3, 2023
View reviewed changes
...s/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java
DefaultBackgroundThread backgroundThread = mockBackgroundThread();
backgroundThread.start();
assertTrue(backgroundThread.isRunning());
TestUtils.waitForCondition(backgroundThread::isRunning, "Failed awaiting for the background thread to be running");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will wait up to 15s, the default timeout. It should be good enough!

machi1990
machi1990 commented May 3, 2023
View reviewed changes
...s/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java
Comment on lines +94 to +95
when(coordinatorManager.poll(anyLong())).thenReturn(mockPollCoordinatorResult());
when(commitManager.poll(anyLong())).thenReturn(mockPollCommitResult());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These stubs call where missing thus causing the NPE as indicated in the JIRA

@machi1990
Copy link
Contributor Author

@philipnee can you've a look? Thanks

kirktrue
kirktrue reviewed May 3, 2023
View reviewed changes
Copy link
Contributor

@kirktrue kirktrue 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 fix!

...s/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java
backgroundThread.close();
assertFalse(backgroundThread.isRunning());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that isRunning() could take some time to return false or is it guaranteed to be set by the time close() is finished?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @kirktrue

It is guaranteed that is running will be false when close() is called:

kafka/clients/src/main/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThread.java

Line 230 in 6c3e82f

this.running = false;

@machi1990 machi1990 requested a review from kirktrue May 3, 2023 16:15
philipnee
philipnee approved these changes May 3, 2023
View reviewed changes
Copy link
Collaborator

@philipnee philipnee left a comment

Choose a reason for hiding this comment

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

Hey thanks for the fix!

@machi1990
Copy link
Contributor Author

Thanks for the review @philipnee Do you know which committer we can tag for this to be reviewed and then merged by them?

@philipnee
Copy link
Collaborator

Hey @machi1990 - I'll get someone to review this. Thanks.

vvcephei
vvcephei approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @machi1990

@vvcephei
Copy link
Contributor

vvcephei commented May 5, 2023

I'm re-running the tests to see if we can get a green build.

@philipnee
Copy link
Collaborator

Thanks @vvcephei - I think this is a flaky test?

Build / JDK 17 and Scala 2.13 / testDescribeStateOfExistingGroupWithRoundRobinAssignor() – kafka.admin.DescribeConsumerGroupTest 2s Skipped - 123

@vvcephei
Copy link
Contributor

vvcephei commented May 8, 2023

Thanks, @philipnee !

I agree it's probably flaky, but I don't see a ticket filed for it. We haven't been very strict recently about this, but I'm reluctant to just forge ahead at this point, given all the mailing list discussions about recent merges breaking the build because they didn't block on failing tests.

Do you mind filing a ticket for this test, and then we can merge it?

(see https://issues.apache.org/jira/browse/KAFKA-8706?jql=project%20%3D%20KAFKA%20AND%20status%20in%20(Open%2C%20%22In%20Progress%22%2C%20Reopened%2C%20%22Patch%20Available%22)%20AND%20text%20~%20%22DescribeConsumerGroupTest%22 )

@philipnee
Copy link
Collaborator

Thank you @vvcephei - https://issues.apache.org/jira/browse/KAFKA-14977 is filed 💘

@machi1990
Copy link
Contributor Author

Thanks @philipnee this should be good to be merged @vvcephei thanks!

@machi1990
Copy link
Contributor Author

Hi @vvcephei @showuon can you've a look at this? Thanks

@philipnee philipnee added the ctr label May 25, 2023
@vvcephei vvcephei merged commit c14e0df into apache:trunk May 26, 2023
1 check failed
@machi1990 machi1990 deleted the KAFKA-14961 branch May 26, 2023 13:32
@machi1990
Copy link
Contributor Author

Thanks @philipnee @vvcephei for the review and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vvcephei vvcephei vvcephei approved these changes

@philipnee philipnee philipnee approved these changes

@kirktrue kirktrue Awaiting requested review from kirktrue

Assignees
No one assigned
Labels
ctr
Projects
None yet
Milestone
No milestone
4 participants
@machi1990 @philipnee @vvcephei @kirktrue