Skip to content

KAFKA-14595 ReassignPartitionsUnitTest rewritten in java#14355

Merged
showuon merged 24 commits intoapache:trunkfrom
nizhikov:KAFKA-14595_tests
Sep 23, 2023
Merged

KAFKA-14595 ReassignPartitionsUnitTest rewritten in java#14355
showuon merged 24 commits intoapache:trunkfrom
nizhikov:KAFKA-14595_tests

Conversation

@nizhikov
Copy link
Copy Markdown
Contributor

@nizhikov nizhikov commented Sep 7, 2023

This PR is part of #13247
It contains changes to rewrite single test in java.
Intention is reduce changes in parent PR.

Committer Checklist (excluded from commit message)

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

@nizhikov nizhikov mentioned this pull request Sep 8, 2023
Merged
3 tasks
tledkov
tledkov reviewed Sep 18, 2023
View reviewed changes
Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
tledkov
tledkov reviewed Sep 18, 2023
View reviewed changes
Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
@nizhikov
Copy link
Copy Markdown
Contributor Author

nizhikov commented Sep 18, 2023 •
edited
Loading

Hello, @showuon .

Are you ready to join the review?
This patch is part of #13247

Big PR contains PartitionReassignCommand rewritten in java.
This PR contains single test ReassignPartitionsUnitTest rewritten in java.
To goal of splitting is to reduce review scope.

Changes are relatively simple.

Please, join the review.

tledkov
tledkov reviewed Sep 18, 2023
View reviewed changes
Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
tledkov
tledkov reviewed Sep 18, 2023
View reviewed changes
Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
tledkov
tledkov approved these changes Sep 18, 2023
View reviewed changes
Copy link
Copy Markdown

@tledkov tledkov left a comment

Choose a reason for hiding this comment

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

The patch is OK with me.
New java-test looks similar to the removed scala-test.

@showuon
Copy link
Copy Markdown
Member

showuon commented Sep 19, 2023

Will review it this week.

@showuon showuon self-assigned this Sep 19, 2023
showuon
showuon reviewed Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@nizhikov , sorry for the late review. Left some comments.

Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
Comment on lines +94 to +104
@BeforeEach
public void setUp() {
Exit.setExitProcedure((statusCode, message) -> {
throw new IllegalArgumentException(message);
});
}

@AfterEach
public void tearDown() {
Exit.resetExitProcedure();
}
Copy link
Copy Markdown
Member

@showuon showuon Sep 22, 2023

Choose a reason for hiding this comment

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

Could we do that in @BeforeAll and @AfterAll?

Copy link
Copy Markdown
Contributor Author

@nizhikov nizhikov Sep 22, 2023

Choose a reason for hiding this comment

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

Yes. Fixed.

Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
));

assertEquals(asScala(expStates), actual._1);
assertEquals(true, actual._2);
Copy link
Copy Markdown
Member

@showuon showuon Sep 22, 2023

Choose a reason for hiding this comment

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

assertTrue ?

Copy link
Copy Markdown
Contributor Author

@nizhikov nizhikov Sep 22, 2023

Choose a reason for hiding this comment

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

Fixed

Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
));

assertEquals(asScala(expStates), actual._1);
assertEquals(false, actual._2);
Copy link
Copy Markdown
Member

@showuon showuon Sep 22, 2023

Choose a reason for hiding this comment

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

assertFalse

Copy link
Copy Markdown
Contributor Author

@nizhikov nizhikov Sep 22, 2023

Choose a reason for hiding this comment

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

Fixed

showuon
showuon reviewed Sep 22, 2023
View reviewed changes
Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java Outdated
}

@SuppressWarnings({"deprecation", "unchecked"})
private static <T> scala.collection.mutable.Set<T> mset(final T...set) {
Copy link
Copy Markdown
Member

@showuon showuon Sep 22, 2023

Choose a reason for hiding this comment

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

Could we rename this helper method to a readable name? Ex: mutableSet?

Copy link
Copy Markdown
Contributor Author

@nizhikov nizhikov Sep 22, 2023

Choose a reason for hiding this comment

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

Yes. renamed

Comment thread tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsUnitTest.java
Comment on lines +182 to +188
Tuple2<scala.collection.Map<TopicPartition, ReassignPartitionsCommand.PartitionReassignmentState>, Object> actual =
findPartitionReassignmentStates(adminClient, seq(
new Tuple2<>(new TopicPartition("foo", 0), seq(0, 1, 3)),
new Tuple2<>(new TopicPartition("foo", 1), seq(1, 2, 3))
));

assertEquals(asScala(expStates), actual._1);
Copy link
Copy Markdown
Member

@showuon showuon Sep 22, 2023

Choose a reason for hiding this comment

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

I can see this is uneasy to convert these lines. Thanks for the effort.

@nizhikov
Copy link
Copy Markdown
Contributor Author

nizhikov commented Sep 22, 2023

@showuon Thank you for the review! I resolve all your comments. Please, take a look one more time

showuon
showuon approved these changes Sep 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@showuon showuon 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 for the PR! Will wait until CI tests completed.

@nizhikov
Copy link
Copy Markdown
Contributor Author

nizhikov commented Sep 22, 2023

@showuon Looks like tests are OK.

@showuon
Copy link
Copy Markdown
Member

showuon commented Sep 23, 2023

Failed tests are unrelated:

    Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
    Build / JDK 17 and Scala 2.13 / integration.kafka.server.FetchFromFollowerIntegrationTest.testRackAwareRangeAssignor()
    Build / JDK 17 and Scala 2.13 / kafka.server.DelegationTokenRequestsTest.testDelegationTokenRequests(String).quorum=kraft
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testSendOffsetsWithGroupMetadata(String).quorum=kraft
    Build / JDK 20 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[2] tlsProtocol=TLSv1.2, useInlinePem=true
    Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
    Build / JDK 20 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
    Build / JDK 20 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft
    Build / JDK 20 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[1] Type=Raft-Combined, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.7-IV0, Security=PLAINTEXT

@showuon showuon merged commit daf8a0d into apache:trunk Sep 23, 2023
@nizhikov nizhikov mentioned this pull request Sep 29, 2023
Merged
3 tasks
rreddy-22 pushed a commit to rreddy-22/kafka-rreddy that referenced this pull request Oct 3, 2023
@nizhikov @rreddy-22
KAFKA-14595 ReassignPartitionsUnitTest rewritten in java (apache#14355)
93103df 
This PR is part of apache#13247
It contains changes to rewrite single test in java.
Intention is reduce changes in parent PR.

Reviewers: Luke Chen <showuon@gmail.com>, Taras Ledkov <tledkov@apache.org>
k-wall pushed a commit to k-wall/kafka that referenced this pull request Nov 21, 2023
@nizhikov @k-wall
KAFKA-14595 ReassignPartitionsUnitTest rewritten in java (apache#14355)
7c14861 
This PR is part of apache#13247
It contains changes to rewrite single test in java.
Intention is reduce changes in parent PR.

Reviewers: Luke Chen <showuon@gmail.com>, Taras Ledkov <tledkov@apache.org>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
@nizhikov @AnatolyPopov
KAFKA-14595 ReassignPartitionsUnitTest rewritten in java (apache#14355)
2e84fd3 
This PR is part of apache#13247
It contains changes to rewrite single test in java.
Intention is reduce changes in parent PR.

Reviewers: Luke Chen <showuon@gmail.com>, Taras Ledkov <tledkov@apache.org>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Feb 22, 2024
@nizhikov @Cerchie
KAFKA-14595 ReassignPartitionsUnitTest rewritten in java (apache#14355)
b5db0ef 
This PR is part of apache#13247
It contains changes to rewrite single test in java.
Intention is reduce changes in parent PR.

Reviewers: Luke Chen <showuon@gmail.com>, Taras Ledkov <tledkov@apache.org>
ray841
ray841 reviewed Jan 25, 2025
View reviewed changes
Copy link
Copy Markdown

@ray841 ray841 left a comment

Choose a reason for hiding this comment

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

code are correct

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

Reviewers

@showuon showuon showuon approved these changes

+2 more reviewers

@ray841 ray841 ray841 left review comments

@tledkov tledkov tledkov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@showuon showuon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nizhikov @showuon @tledkov @ray841