KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder #16140

brenden20 · 2024-05-30T14:50:49Z

Completely migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder and removed all usages of spy objects and replaced with mocks. Removed testEnsureMetadataUpdateOnPoll() since it was doing integration testing. Also I added 2 new tests to get more complete test coverage of ConsumerNetworkThread. All tests are passing

Removed dependencies upon ConsumerTestBuilder

Removed change of toString to toStringBase, moving change to separate branch

Added test for RMs, seeing if pollWaitTimeMs is computed correctly

Cleaning up comments

Added a new param test to ensure ConsumerNetworkThread wait times are being computed correctly.

Removed all redundant/integration tests

Changed variables names in testConsumerNetworkThreadWaitTimeComputations to be more descriptive

Removed vars and methods with no usage

Removed unused imports

Updating comments

Comments

Updated constructor to include correct parameters

Added previously removed tests and changed some imports and edited the constructor

Did some minor updates, changed testMetadataUpdateEvent since it was integration testing

Updated constructor, removed unnecessary variables. Also updated a few tests to make them work or to make them unit tests rather than integration tests

Updated comment on testEnsureMetadataUpdateOnPoll

Remove comment

Added testEnsureCloseStopsRunningThread. Updated testConsumerNetworkThreadWaitTimeComputations to also test that networkClientDelegate polls with correct times. Removed one instance of a spy object, replaced with mock.

Cleaning up warnings, changed lambda expressions to explicit method references

Removed tests from this test that seem to be more like integration testing

Removed testEnsureMetadataUpdateOnPoll(). This is an integration test, tried many different ways to still get it to pass but could not. The issue is that for the metadata. updateWithCurrentRequestVersion() method to be invoked, many objects must not be mocks. This causes issues when trying to use verify(), as it can only take a mock, not an actual instantiated object. I also cleaned up the code a little bit

Changed instantiation of MockClient to not include metadata in constructor

Moved all code from ConsumerNetworkThreadUnitTest to ConsumerNetworkThreadTest and removed ConsumerNetworkThreadUnitTest

Comment removal

kirktrue

Thanks @brenden20. This is a welcome change and I only have minor nits at this point.

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

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

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

kirktrue · 2024-05-30T19:34:18Z

Looks like there are some checkstyle failures due to the use of wildcard imports.

kirktrue

Left some suggestions as to the checkstyle issues.

You should be able to run this command locally to verify the checkstyle rules pass:

./gradlew check -x test

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

brenden20 · 2024-06-07T19:17:32Z

@lianetm implemented your suggestions on the description and testConsumerNetworkThreadPollTimeComputations(). Let me know how it looks

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

Updates testConsumerNetworkThreadPollTimeComputations() and fixed a checkstyle error

brenden20 · 2024-06-07T20:59:38Z

@lianetm I implemented your new suggestions, let me know if it looks good

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

lianetm

Thanks for changes @brenden20 , this LGTM.

Really nice how you got rid of the heavy use of unneeded spy objects that the ConsumerTestBuilder was bringing in. Thanks!

mjsax

I am not very familiar with the control flow of the consumer, but it seems this PR is doing much more than an "in place" rewrite of the test. Would be good to understand better why so many test methods got removed.

mjsax · 2024-06-07T22:55:29Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

+    }
+
+    @Test
+    public void testEnsureCloseStopsRunningThread() {


Seems this is one of the newly added tests?

mjsax · 2024-06-07T22:55:48Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

+
+    @ParameterizedTest
+    @ValueSource(longs = {ConsumerNetworkThread.MAX_POLL_TIMEOUT_MS - 1, ConsumerNetworkThread.MAX_POLL_TIMEOUT_MS, ConsumerNetworkThread.MAX_POLL_TIMEOUT_MS + 1})
+    public void testConsumerNetworkThreadPollTimeComputations(long exampleTime) {


Seems this is the second newly added tests?

mjsax · 2024-06-07T22:57:02Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

+    public void testConsumerNetworkThreadPollTimeComputations(long exampleTime) {
+        List<Optional<? extends RequestManager>> list = new ArrayList<>();
+        list.add(Optional.of(coordinatorRequestManager));
+        list.add(Optional.of(heartbeatRequestManager));


Why do we only add two of the three request managers to this list?

Really just to declutter, I thought it might make that specific test kind of long if I added every request manager. 2 was the minimum amount necessary to test the functionality of the test. It just needs to return the shortest timeUntilNextPollMs. So in order to test this I just did 2 request managers with different poll times to make sure it was being calculated correctly.

I can add the offsetsRequestManager if needed.

It wouldn't hurt to have all managers, but agree that having just a subset > 1 should be enough, because the logic is not specific to the managers (the consumerNetworkThread is blind on the managers, it only knows it has a set of them wrapped in a RequestManagers collection)

mjsax · 2024-06-07T22:59:23Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

@@ -148,35 +169,32 @@ public void testStartupAndTearDown() throws InterruptedException {
    }

    @Test
-    public void testApplicationEvent() {


It seems this test is rewritten as testApplicationEventIsProcessed further below? And testRequestsTransferFromManagersToClientOnThreadRun is another newly added test?

Yes, this test as well as many of the other application event tests were rewritten into a single parameterized test. It gets the parameters from applicationEvents(), where it supplies a stream of different kinds of events. This was done to condense the code a bit.

And testRequestsTransferFromManagersToClientOnThreadRun() is a new test

mjsax · 2024-06-07T23:01:45Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

    }

-    @Test
-    public void testMetadataUpdateEvent() {


Seems this test was effectively removed? Why?

Refer to previous comment for all event tests that were removed

mjsax · 2024-06-07T23:01:53Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java


-    @Test
-    public void testAsyncCommitEvent() {


Seems this test was effectively removed? Why?

mjsax · 2024-06-07T23:01:59Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java


-    @Test
-    public void testSyncCommitEvent() {


Seems this test was effectively removed? Why?

mjsax · 2024-06-07T23:02:07Z

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

@@ -190,15 +208,6 @@ public void testListOffsetsEventIsProcessed(boolean requireTimestamp) {
        assertTrue(applicationEventsQueue.isEmpty());
    }

-    @Test
-    public void testResetPositionsEventIsProcessed() {


Seems this test was effectively removed? Why?

Same for more test below

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java

philipnee

hi @brenden20 - i left a comment, otherwise lgtm.

brenden20 · 2024-06-12T14:01:44Z

@mjsax can you take a look?

mjsax · 2024-06-13T20:35:49Z

Thanks for the PR @brenden20! Merged to trunk.

brenden20 · 2024-06-13T20:40:00Z

@mjsax thank you for the review!

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 <[email protected]>

brenden20 and others added 27 commits May 24, 2024 12:08

Migrated ConsumerTestBuilder

f82d15c

Removed dependencies upon ConsumerTestBuilder

Removed changes to OffsetFetchRequestState

c7a479e

Removed change of toString to toStringBase, moving change to separate branch

Added test for RMs

1fcf205

Added test for RMs, seeing if pollWaitTimeMs is computed correctly

Cleaning up comments

5271bb5

Cleaning up comments

Added test for wait time computation

3f037e5

Added a new param test to ensure ConsumerNetworkThread wait times are being computed correctly.

Test removal

4a85031

Removed all redundant/integration tests

Changed variables names

a10a961

Changed variables names in testConsumerNetworkThreadWaitTimeComputations to be more descriptive

Removed vars and methods with no usage

a1a4de0

Removed vars and methods with no usage

Removed unused imports

372eea6

Removed unused imports

Updating comments

43be21e

Updating comments

Comments

ce3d5c5

Comments

Merge branch 'apache:trunk' into 16001

ad9d38b

Merge branch 'apache:trunk' into 16001

1fccaa8

Updated constructor

8a785e1

Updated constructor to include correct parameters

Added tests back

70ad457

Added previously removed tests and changed some imports and edited the constructor

Minor updates to tests

94f9bcc

Did some minor updates, changed testMetadataUpdateEvent since it was integration testing

Constructor and test updates

99c4e15

Updated constructor, removed unnecessary variables. Also updated a few tests to make them work or to make them unit tests rather than integration tests

Updated comment

9aa6e53

Updated comment on testEnsureMetadataUpdateOnPoll

Merge branch '16001' of https://github.com/brenden20/kafka into 16001

0a502aa

Remove comment

a587124

Remove comment

Added testEnsureCloseStopsRunningThread(), updated others

a95e0c7

Added testEnsureCloseStopsRunningThread. Updated testConsumerNetworkThreadWaitTimeComputations to also test that networkClientDelegate polls with correct times. Removed one instance of a spy object, replaced with mock.

Cleaning up warnings

da90e1b

Cleaning up warnings, changed lambda expressions to explicit method references

Updated testAssignmentChangeEvent()

8c9ad01

Removed tests from this test that seem to be more like integration testing

Changed MockClient

5897bed

Changed instantiation of MockClient to not include metadata in constructor

Moved all changes into original file

2402e6f

Moved all code from ConsumerNetworkThreadUnitTest to ConsumerNetworkThreadTest and removed ConsumerNetworkThreadUnitTest

Comment removal

91d5266

Comment removal

kirktrue suggested changes May 30, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

Implement suggestion

7c0aa57

lianetm reviewed Jun 7, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

lianetm reviewed Jun 7, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

lianetm reviewed Jun 7, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

lianetm reviewed Jun 7, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

Updated testConsumerNetworkThreadPollTimeComputations()

0236a18

Updates testConsumerNetworkThreadPollTimeComputations() and fixed a checkstyle error

lianetm reviewed Jun 7, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

Fixed error

41948b7

lianetm approved these changes Jun 7, 2024

View reviewed changes

mjsax reviewed Jun 7, 2024

View reviewed changes

philipnee reviewed Jun 9, 2024

View reviewed changes

...nts/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkThreadTest.java Outdated Show resolved Hide resolved

philipnee approved these changes Jun 9, 2024

View reviewed changes

brenden20 and others added 7 commits June 10, 2024 10:40

Merge branch 'apache:trunk' into 16001

609fd0c

Implemented suggestion

61f673f

Merge branch '16001' of https://github.com/brenden20/kafka into 16001

dcf2dab

Merge branch 'apache:trunk' into 16001

9bcc67d

Merge branch 'apache:trunk' into 16001

beaa017

Merge branch 'apache:trunk' into 16001

aac3cd9

Merge branch 'apache:trunk' into 16001

4aad5c6

Merge branch 'apache:trunk' into 16001

8b0807a

mjsax changed the title ~~KAFKA-16001 Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder~~ KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder Jun 13, 2024

mjsax merged commit a0b716e into apache:trunk Jun 13, 2024
1 check was pending

brenden20 deleted the 16001 branch June 13, 2024 20:37

brenden20 mentioned this pull request Jul 25, 2024

MINOR: Remove ConsumerTestBuilder.java #16691

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder #16140

KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder #16140

brenden20 commented May 30, 2024 •

edited

Loading

kirktrue left a comment

kirktrue commented May 30, 2024

kirktrue left a comment

brenden20 commented Jun 7, 2024

brenden20 commented Jun 7, 2024

lianetm left a comment

mjsax left a comment

mjsax Jun 7, 2024

brenden20 Jun 7, 2024

mjsax Jun 7, 2024

brenden20 Jun 7, 2024

mjsax Jun 7, 2024

brenden20 Jun 7, 2024

lianetm Jun 12, 2024 •

edited

Loading

mjsax Jun 7, 2024

brenden20 Jun 7, 2024

mjsax Jun 7, 2024

brenden20 Jun 7, 2024

mjsax Jun 7, 2024

mjsax Jun 7, 2024

mjsax Jun 7, 2024

philipnee left a comment

brenden20 commented Jun 12, 2024

mjsax commented Jun 13, 2024

brenden20 commented Jun 13, 2024

KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder #16140

KAFKA-16001: Migrated ConsumerNetworkThreadTest away from ConsumerTestBuilder #16140

Conversation

brenden20 commented May 30, 2024 • edited Loading

kirktrue left a comment

Choose a reason for hiding this comment

kirktrue commented May 30, 2024

kirktrue left a comment

Choose a reason for hiding this comment

brenden20 commented Jun 7, 2024

brenden20 commented Jun 7, 2024

lianetm left a comment

Choose a reason for hiding this comment

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lianetm Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipnee left a comment

Choose a reason for hiding this comment

brenden20 commented Jun 12, 2024

mjsax commented Jun 13, 2024

brenden20 commented Jun 13, 2024

brenden20 commented May 30, 2024 •

edited

Loading

lianetm Jun 12, 2024 •

edited

Loading