KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java by nizhikov · Pull Request #14456 · apache/kafka

nizhikov · 2023-09-26T20:05:10Z

This PR is part of #13247
It contains ReassignPartitionsIntegrationTest rewritten in java.
Goal of PR is reduce changes size in main PR.

Committer Checklist (excluded from commit message)

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

nizhikov · 2023-09-27T17:06:15Z

Hello, @showuon
This PR is part of #13247
This is last test of ReassignPartitionCommand in scala.
If you have some time, please, join the review.

nizhikov · 2023-09-28T16:39:49Z

Hello @tledkov can you, please, take a look?

tledkov

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

It would be nice to draw the community's attention to the fact that the functionality of ClusterTestExtensions is insufficient. The @BeforeEach cannot use with a started cluster. And there is not similar fucntionality. In this test, for example, this is the reason for copying the call of the method createTopics in each test.

jolshan · 2023-09-29T16:11:22Z

+
+    private Admin adminClient;
+
+    @ClusterTemplate("zkAndKRaft")


Can we keep with the @ParameterizedTest format we use for other tests? I think it is good to keep things consistent.

Also let me know if this was decided for the Java migrations and I missed that. 😅

Hello, @jolshan

For now, I know the following ways to setup integration test

Use scala QuorumTestHarness - see existing ReassignPartitionsIntegrationTest.scala.

Use java annotation @ClusterTestDefaults - see DeleteRecordsCommandTest.

Use @ClusterTemplate - like I did in ReassignPartitionsIntegrationTest.java for now.

I don't know what is the correct way to implement test.
AFAICU you suggesting to use QuorumTestHarness with ParametrizedTest annotation.
Is this correct?

I had such version of ReassignPartitionsIntegrationTest.java but replace it with the current version. So it's will be easy to revert to earlier version :)

I was suggesting the QuorumTestHarness approach, but I wasn't sure if there was a unified/preferred way to do the conversions. Unless there are any issues with the QuorumTestHarness approach or there are benefits with the ClusterTemplate one, I think it makes sense to stick with it.

I reverted new ReassignPartitionsIntegrationTest to use QuorumTestHarness so now it's structure is very similary to the scala version. Please, take a look.

jolshan

Thanks @nizhikov. A few comments

jolshan · 2023-09-29T18:19:14Z

+
+    @SuppressWarnings({"deprecation", "unchecked"})
+    private static <T> scala.collection.mutable.Set<T> mutableSet(final T...set) {
+        return JavaConverters.asScalaSet(new HashSet<>(Arrays.asList(set)));


I thought we had some non-deprecated methods to do this in the code. Is this no longer the case?

Is this no longer the case?

I spend some time to find one during implementation of #14355 , actually.
All methods I found either don't exists in scala 2.12 or deprecated in scala 2.13.

Do you know the way to convert collections scala <-> java that will work both in scala 2.12 and scala 2.13 and will not be deprecated in scala 2.13?

Anyway, these methods will be removed in #13247 .

Hmm. I think I saw some examples of scala.collection.JavaConverters that didn't have deprecation warnings on them. I did a search of asScala in the java files and looked if the files had deprecation warnings. There were a few that seemed to not.

If we remove these, then maybe it's ok. We should file a JIRA to clean them up though.

We should file a JIRA to clean them up though

Jira not required - because in #13247 I will remove ReassignPartitionCommand.scala therefore all POJO inside it.
So all tests will forcefully use java versions of clasees.

The goal of this PR is to simplify review of the single test conversion and reduce changes in #13247

jolshan · 2023-09-29T18:24:44Z

+        }
+    }
+
+    private ReassignPartitionsCommand.VerifyAssignmentResult asScala(VerifyAssignmentResult res) {


I'm not sure how I feel about these asScala methods. I'm wondering if there is any way to give a better name or make this cleaner.

Will these also be removed in the other PR you mentioned?

Yes. asScala methods will be removed in #13247

nizhikov · 2023-09-29T22:33:00Z

@jolshan Thanks for the review. I resolve all your comments. Please, take a look one more time.

nizhikov · 2023-09-30T06:14:07Z

@jolshan I answered all your question. Take a look, please.

nizhikov · 2023-10-02T10:34:22Z

CI is OK after code review changes.
@jolshan do you have other comments?

jolshan · 2023-10-02T20:10:24Z

Hey sorry. I was off for the weekend. I can take another look.

jolshan

Thanks Nikolay!

nizhikov · 2023-10-02T20:33:19Z

@jolshan Thank you very much for the review and merge. Appreciate it.

msn-tldr · 2023-10-03T15:09:41Z

    testImplementation project(':connect:api')
    testImplementation project(':connect:runtime')
    testImplementation project(':connect:runtime').sourceSets.test.output
+    testImplementation project(':storage:api').sourceSets.main.output


@nizhikov
This gradle change is breaking trunk builds for me locally, should this be :storage:storage-api instead?

I see builds fail on jenkins similarly -
https://lists.apache.org/thread/qnscvw8nb3dscrkm43r7g5224067jzc6

Hello.

Please, take a look at jenkins build of trunk after this merged - https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka/detail/trunk/2248/pipeline

Looks like it builds successfully.
Can you, please, clarify, how you trying to build trunk and what exactly fails?

It seems like 7553d3f breaks compilation.

I will provide fix in the nearest time.

Fix - #14475

@msn-tldr Fix merged e90f82b

I was worried for a moment since I definitely checked the build on this 😅 Glad it was sorted.

@nizhikov i can see its fixed in trunk, thanks.

…he#14456) This PR is part of apache#13247 It contains ReassignPartitionsIntegrationTest rewritten in java. Goal of PR is reduce changes size in main PR. Reviewers: Taras Ledkov <tledkov@apache.org>, Justine Olshan <jolshan@confluent.io>

nizhikov added 7 commits September 26, 2023 23:04

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

a6eeeca

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

608f415

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

344c17b

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

90df2e6

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

4c58add

KAFKA-14595 ReassignPartitionsIntegrationTest rewritten in java

fd29381

KAFKA-14595 Reduce classpath changes.

e226fd9

nizhikov mentioned this pull request Sep 27, 2023

KAFKA-14595 Move ReassignPartitionsCommand to java #13247

Merged

3 tasks

nizhikov added 3 commits September 29, 2023 17:13

KAFKA-14595 Code review fixes.

9cfcf5b

KAFKA-14595 Code review fixes.

5b5ff80

KAFKA-14595 Code review fixes.

809ccf8

tledkov reviewed Sep 29, 2023

View reviewed changes

KAFKA-14595 Code review fixes.

cd2c128

tledkov approved these changes Sep 29, 2023

View reviewed changes

jolshan reviewed Sep 29, 2023

View reviewed changes

nizhikov added 2 commits September 29, 2023 20:20

KAFKA-14595 Revert Integration test to QuorumTestHarness

1bad17d

KAFKA-14595 Code style fixes

ebf9bf3

jolshan reviewed Sep 29, 2023

View reviewed changes

nizhikov added 2 commits September 30, 2023 01:23

KAFKA-14595 Code review fixes.

14a990a

KAFKA-14595 Import control fix

7d4ab77

jolshan approved these changes Oct 2, 2023

View reviewed changes

jolshan merged commit 8f8dbad into apache:trunk Oct 2, 2023

msn-tldr reviewed Oct 3, 2023

View reviewed changes

Conversation

nizhikov commented Sep 26, 2023

Committer Checklist (excluded from commit message)

Uh oh!

nizhikov commented Sep 27, 2023

Uh oh!

nizhikov commented Sep 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tledkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolshan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizhikov Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Sep 29, 2023

Uh oh!

nizhikov commented Sep 30, 2023

Uh oh!

nizhikov commented Oct 2, 2023

Uh oh!

jolshan commented Oct 2, 2023

Uh oh!

jolshan left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Oct 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizhikov Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizhikov Sep 30, 2023 •

edited

Loading

nizhikov Oct 3, 2023 •

edited

Loading