Skip to content

KAFKA-15999: Migrate HeartbeatRequestManagerTest away from ConsumerTestBuilder#16200

Merged
chia7712 merged 84 commits intoapache:trunkfrom
brenden20:15999
Jul 25, 2024
Merged

KAFKA-15999: Migrate HeartbeatRequestManagerTest away from ConsumerTestBuilder#16200
chia7712 merged 84 commits intoapache:trunkfrom
brenden20:15999

Conversation

@brenden20
Copy link
Copy Markdown
Contributor

@brenden20 brenden20 commented Jun 4, 2024

In this PR I have completely migrated HeartbeatRequestManagerTest away from ConsumerTestBuilder. I have removed any instances of spy objects and used mocks wherever possible. 31/31 tests are passing. I also removed one test that existed in another test file.

brenden20 added 14 commits June 3, 2024 16:14
@brenden20
Revert "KAFKA-16713: Define initial set of RPCs for KIP-932 (apache#1…
d6bff1f 
…6022)"

This reverts commit 8f82f14.
@brenden20
Various test changes
23e7f97
@brenden20
Stylistic changes
f7c5ea8
@brenden20
Variable addition and cleanup
8d5686b
@brenden20
Test updates
c0404dc 
Updated testFencedMemberStopHeartbeatUntilItReleasesAssignmentToRejoin() and testSuccessfulHeartbeatTiming() to get them to pass
@brenden20
Removed testHeartbeatMetrics()
a719c95 
Removed testHeartbeatMetrics(), only test I was unable to get to pass. Also found that this test already exists in HeartbeatMetricsManagerTest
@brenden20
Cleaning up
f291b50
@brenden20
Reapply "KAFKA-16713: Define initial set of RPCs for KIP-932 (apache#…
c96499a 
…16022)"

This reverts commit d6bff1f.
@brenden20
Cleaning up heartbeatRequestState vars
2f6a10e
@brenden20
Cleaning up membershipManager vars
29d7795
@brenden20
Cleaning up heartbeatRequestManager vars
6bbf43a
@brenden20
More cleanup
e35e0af
@brenden20
Whitespace fixes
6ea3537
@brenden20
Update HeartbeatRequestManagerTest.java
1d4df14
@brenden20
Copy link
Copy Markdown
Contributor Author

brenden20 commented Jun 5, 2024 •
edited
Loading

@philipnee @lianetm @kirktrue this is ready for review. Let me know of any suggestions!

@brenden20 brenden20 marked this pull request as ready for review June 6, 2024 20:17
@lianetm
Copy link
Copy Markdown
Member

lianetm commented Jul 18, 2024

Hey @brenden20 , thanks for the updates! Just a few minor comments left.

@brenden20
Copy link
Copy Markdown
Contributor Author

brenden20 commented Jul 19, 2024

@lianetm thank you for the review! I addressed the comments you left. Let me know if it is good

lianetm
lianetm approved these changes Jul 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@lianetm lianetm left a comment

Choose a reason for hiding this comment

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

Thanks @brenden20, great improvement! LGTM.

@mjsax mjsax changed the title KAFKA-15999 Migrate HeartbeatRequestManagerTest away from ConsumerTestBuilder KAFKA-15999: Migrate HeartbeatRequestManagerTest away from ConsumerTestBuilder Jul 20, 2024
@philipnee philipnee added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Jul 23, 2024
@lianetm
Copy link
Copy Markdown
Member

lianetm commented Jul 23, 2024

Hey @chia7712, would you maybe have some time to take a look at this? Nice improvement @brenden20 did on the HeartbeatRequestManager test. Thanks!

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jul 23, 2024

I will take a look today!

chia7712
chia7712 reviewed Jul 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@brenden20 thanks for this great patch. I have only one small comment for you. PTAL

Comment thread ...s/src/test/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManagerTest.java Outdated
@brenden20
Copy link
Copy Markdown
Contributor Author

brenden20 commented Jul 24, 2024

@chia7712 thank you for your feedback! I have addressed your comment by moving those object creations into a helper method. Let me know how it looks!

@chia7712 chia7712 mentioned this pull request Jul 24, 2024
Merged
chia7712
chia7712 reviewed Jul 24, 2024
View reviewed changes
Comment thread ...s/src/test/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManagerTest.java Outdated
if (testBuilder != null) {
testBuilder.close();
}
private void createHeartbeatStateandRequestManager() {
Copy link
Copy Markdown
Member

@chia7712 chia7712 Jul 24, 2024

Choose a reason for hiding this comment

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

nit: createHeartbeatStateandRequestManager -> createHeartbeatStateAndRequestManager

Copy link
Copy Markdown
Contributor Author

@brenden20 brenden20 Jul 24, 2024

Choose a reason for hiding this comment

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

Whoops, missed that. Fixed now.

@brenden20
Copy link
Copy Markdown
Contributor Author

brenden20 commented Jul 24, 2024

@chia7712 fixed that small issue with the name. Let me know if there is anything else!

@chia7712 chia7712 merged commit cca0390 into apache:trunk Jul 25, 2024
@brenden20 brenden20 deleted the 15999 branch July 25, 2024 15:43
@brenden20 brenden20 mentioned this pull request Jul 25, 2024
Merged
3 tasks
abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 2024
@brenden20 @abhi-ksolves
KAFKA-15999 Migrate HeartbeatRequestManagerTest away from ConsumerTes…
f28b653 
…tBuilder (apache#16200)

In this PR I have completely migrated HeartbeatRequestManagerTest away from ConsumerTestBuilder. I have removed any instances of spy objects and used mocks wherever possible. 31/31 tests are passing. I also removed one test that existed in another test file.

Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
chia7712 pushed a commit that referenced this pull request Aug 5, 2024
@brenden20
MINOR: Remove ConsumerTestBuilder.java (#16691)
0b78d84 
The purpose of this PR is to remove ConsumerTestBuilder.java since it is no longer needed. The following PRs have eliminated the use of ConsumerTestBuilder:
#14930
#16140
#16200
#16312

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

@lianetm lianetm lianetm approved these changes

+1 more reviewer

@philipnee philipnee philipnee left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

consumer KIP-848 The Next Generation of the Consumer Rebalance Protocol tests Test fixes (including flaky tests)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@brenden20 @lianetm @chia7712 @philipnee @mjsax