-
Notifications
You must be signed in to change notification settings - Fork 14.7k
MINOR: Move topic creation before consumer creation in testListGroups integration test #20496
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
MINOR: Move topic creation before consumer creation in testListGroups integration test #20496
Conversation
The test is still flaky on my local machine. Perhaps it needs |
Added a waitUtilTrue to wait until all groups are stable before validating the listGroup contents. |
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 fix. I looped this test 50 times locally and didn’t see any failures.
Also left a small suggestion.
filteredClassicGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredConsumerGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredShareGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredClassicGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && |
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.
filteredClassicGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) &&
It looks like this line is duplicated.
…egrationtest-listgroups
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.
LGTM
Merged #20496 into trunk |
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.
@bbejeck Could you please use committer-tools/reviewers.py
to format the reviewers before merging the PR?
filteredClassicGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredConsumerGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredShareGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) && | ||
filteredStreamsGroups.forall(_.groupState().orElse(null) == GroupState.STABLE) |
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.
Should we also add the check for simple group?
This PR moves the topic creation before consumer creations in
PlaintextAdminIntegrationTest.testListGroups
, to avoid potentialerrors if consumer creates topic due to metadata update.
See discussion
#20244 (comment)
Reviewers: @chia7712, [email protected]