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-14133: Migrate Admin mock in TaskManagerTest to Mockito #13712

Merged
merged 2 commits into from Jun 13, 2023
Merged

KAFKA-14133: Migrate Admin mock in TaskManagerTest to Mockito #13712

merged 2 commits into from Jun 13, 2023

Conversation

clolov
Copy link
Collaborator

@clolov clolov commented May 12, 2023 •
edited

This pull requests migrates the Admin mock in TaskManagerTest from EasyMock to Mockito.
The change is restricted to a single mock to minimize the scope and make it easier for review.

The reasoning as to why we would like to migrate a single mock rather than all mocks in the file at the same time has been discussed in #12607 (comment)

Similar to:
#13529
#13621
#13681
#13711

This needs to be rebased on top of #13711 before merging

machi1990
machi1990 reviewed May 12, 2023
View reviewed changes
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated
Comment on lines 3781 to 3729

verify(adminClient);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is removed, and I wonder whether we should add a similar assertion for the adminClient mock after each of these two calls: https://github.com/apache/kafka/blob/ff6b6dd601d9a6156dba249a8b52201a34eaca13/streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java#L3775-L3779

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Heya, thank you for the review! I am not quite certain I follow what you mean. The reason this is removed here and elsewhere is that Mockito verifies internally that all when statements are called. EasyMock does not do this by default so it requires an explicit verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right; I didn't notice that we are using STRICT_STUBS strictness: https://github.com/apache/kafka/blob/ff6b6dd601d9a6156dba249a8b52201a34eaca13/streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java#L208

Thanks for the reply!

machi1990
machi1990 reviewed May 12, 2023
View reviewed changes
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java
@@ -3816,8 +3811,6 @@ public Map<TopicPartition, Long> purgeableOffsets() {
// so it would fail verification if we invoke the admin client again.
purgableOffsets.put(t1p1, 17L);
taskManager.maybePurgeCommittedRecords();

verify(adminClient);
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as https://github.com/apache/kafka/pull/13712/files#r1192833508

streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java
@@ -3842,8 +3835,6 @@ public void shouldIgnorePurgeDataErrors() {

taskManager.maybePurgeCommittedRecords();
taskManager.maybePurgeCommittedRecords();

verify(adminClient);
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as https://github.com/apache/kafka/pull/13712/files#r1192833508

@divijvaidya divijvaidya added the tests Test fixes (including flaky tests) label May 15, 2023
@clolov clolov added the streams label May 18, 2023
cadonna
cadonna reviewed Jun 6, 2023
View reviewed changes
Copy link
Contributor

@cadonna cadonna 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, @clolov !

Here my feedback.

streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java
Comment on lines +3751 to +3700
when(adminClient.deleteRecords(singletonMap(t1p1, RecordsToDelete.beforeOffset(5L))))
.thenReturn(new DeleteRecordsResult(singletonMap(t1p1, completedFuture())));
when(adminClient.deleteRecords(singletonMap(t1p1, RecordsToDelete.beforeOffset(17L))))
.thenReturn(new DeleteRecordsResult(singletonMap(t1p1, completedFuture())));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to verify the order of the calls since the admin mock is reset to strict in EasyMock. The order is indeed important here because you do not want to purge records in the wrong order from a repartition topic.

@clolov
Copy link
Collaborator Author

clolov commented Jun 6, 2023

Heya @cadonna! I hope I have addressed your comment, rebased and updated the overview!

cadonna
cadonna approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

LGTM!

There is just one checkstyle failure that I tried to correct.

streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java Outdated Show resolved Hide resolved
@cadonna
Copy link
Contributor

cadonna commented Jun 12, 2023

Build failures are unrelated:

    Build / JDK 8 and Scala 2.12 / integration.kafka.server.FetchFromFollowerIntegrationTest.testRackAwareRangeAssignor()
    Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures()
    Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.clients.consumer.KafkaConsumerTest.testShouldAttemptToRejoinGroupAfterSyncGroupFailed()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.DedicatedMirrorIntegrationTest.testMultiNodeCluster()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testReplicationWithEmptyPartition()
    Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
    Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()

@clolov
Copy link
Collaborator Author

clolov commented Jun 12, 2023

Heya @cadonna! Is there something I need to do or this can be merged?

@cadonna
Copy link
Contributor

cadonna commented Jun 12, 2023

I was not sure. The description says

This needs to be rebased on top of #13711 before merging

Can I merge it anyways?

@clolov
Copy link
Collaborator Author

clolov commented Jun 13, 2023

Ah, sorry, yeah, I have caused the confusion. Technically the order of merging between this and #13711 doesn't matter as long as one is rebased on top of the other afterwards. This appears to be ready to merge so let's go with merging this and I will then update 13711.

@cadonna cadonna merged commit 7f0e455 into apache:trunk Jun 13, 2023
1 check failed
@clolov
Copy link
Collaborator Author

clolov commented Jun 13, 2023

Thank you! I will now address the comments on the other one!

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

@machi1990 machi1990 machi1990 left review comments

@cadonna cadonna cadonna approved these changes

Assignees
No one assigned
Labels
streams tests Test fixes (including flaky tests)
Projects
None yet
Milestone
No milestone
4 participants
@clolov @cadonna @machi1990 @divijvaidya