KAFKA-14589 [1/3] Tests of ConsoleGroupCommand rewritten in java by nizhikov · Pull Request #15256 · apache/kafka

nizhikov · 2024-01-24T17:19:51Z

This PR is part of #14471
Is contains some of ConsoleGroupCommand tests rewritten in java.
Intention of separate PR is to reduce changes and simplify review.

Committer Checklist (excluded from commit message)

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

…n in java

showuon · 2024-01-26T12:13:43Z

Will take a look next week. Thanks.

showuon

Thanks for the PR. Left some comments.

showuon · 2024-01-30T12:15:35Z

+
+        String output = kafka.utils.TestUtils.grabConsoleOutput(() -> {
+            service.deleteGroups();
+            return null;


Will it have compiling failure after removing return null?

showuon · 2024-01-30T12:22:11Z

+        System.arraycopy(cgcArgs, 0, cgcArgs2, 0, cgcArgs.length);
+        cgcArgs2[cgcArgs2.length - 2] = "--group";
+        cgcArgs2[cgcArgs2.length - 1] = missingGroup;


I know we did that in scala like this, but after rewriting in Java it becomes more unreadable. Maybe we can just do this:

String[] cgcArgs2 = new String[]{"--bootstrap-server", bootstrapServers(listenerName()), "--delete", "--group", GROUP, "--group", missingGroup};

showuon · 2024-01-30T12:23:25Z

+        String[] cgcArgs2 = new String[cgcArgs.length + 2];
+
+        System.arraycopy(cgcArgs, 0, cgcArgs2, 0, cgcArgs.length);
+        cgcArgs2[cgcArgs2.length - 2] = "--group";
+        cgcArgs2[cgcArgs2.length - 1] = missingGroup;


showuon · 2024-01-30T12:25:20Z

+        };
+    }
+
+    @Test


Should we add these?

@ParameterizedTest(name = TEST_WITH_PARAMETERIZED_QUORUM_NAME) @ValueSource(strings = {"zk", "kraft"})

Same for below tests.

Great catch. Thanks. Fixed.
This was added 2 weeks ago and I missed to reflect that change in java test version.

showuon · 2024-01-30T12:30:50Z

+        scala.collection.Set<ConsumerGroupListing> expectedListing = set(Arrays.asList(
+            new ConsumerGroupListing(simpleGroup, true, Optional.of(ConsumerGroupState.EMPTY)),
+            new ConsumerGroupListing(GROUP, false, Optional.of(ConsumerGroupState.STABLE))));
+
+        final scala.collection.Set[] foundListing = new scala.collection.Set[1];


Could we use java collection here?

It seems that usage of scala collections is more convinient for now.
But, of course, when java version of command will be merged we will use java collections here and everywhere in test. You can see how this don for already mereged tests:

https://github.com/apache/kafka/pull/14471/files#diff-4e22e01d567ea1b5087aa60354fa127bb25b892b430a6b86ab39a10edd393c0a

showuon · 2024-01-30T12:34:08Z

+        result = ConsumerGroupCommand.consumerGroupStatesFromString("stable");
+        assertEquals(set(Arrays.asList(ConsumerGroupState.STABLE)), result);
+
+        result = ConsumerGroupCommand.consumerGroupStatesFromString("stable, assigning");
+        assertEquals(set(Arrays.asList(ConsumerGroupState.STABLE, ConsumerGroupState.ASSIGNING)), result);
+
+        result = ConsumerGroupCommand.consumerGroupStatesFromString("dead,reconciling,");
+        assertEquals(set(Arrays.asList(ConsumerGroupState.DEAD, ConsumerGroupState.RECONCILING)), result);


I didn't see these tests in scala. Did you add them?

It is fresh changes from 82920ff

showuon · 2024-01-30T12:34:20Z

+        assertThrows(IllegalArgumentException.class, () -> ConsumerGroupCommand.consumerGroupStatesFromString("bad, wrong"));
+
+        assertThrows(IllegalArgumentException.class, () -> ConsumerGroupCommand.consumerGroupStatesFromString("  bad, Stable"));
+
+        assertThrows(IllegalArgumentException.class, () -> ConsumerGroupCommand.consumerGroupStatesFromString("   ,   ,"));


It seems 1 test is missing.

Double checked and can't spot missing test. Can you, please, clarify what test are missing?

Ah, there's some update in the test. Please ignore it. Thanks.

showuon · 2024-01-30T12:36:13Z

+        String simpleGroup = "simple-group";
+        addSimpleGroupExecutor(simpleGroup);
+        addConsumerGroupExecutor(1);
+        final String[] out = {""};


Why can't we declare as String directly?
final String out = ""?

out value changed in java lambda

out = grabConsoleOutput() and only final variable (which can't be changed) can be used inside java lambda.

I can rework String array to usage of AtomicReference.
What do you think?

Reworked to AtomicReference.

Looks good!

showuon · 2024-01-30T12:37:19Z

+        String[] cgcArgs3 = new String[]{"--bootstrap-server", bootstrapServers(listenerName()), "--list", "--state", "Stable"};
+        TestUtils.waitForCondition(() -> {
+            out[0] = kafka.utils.TestUtils.grabConsoleOutput(() -> {
+                ConsumerGroupCommand.main(cgcArgs3);
+                return null;
+            });
+            return out[0].contains("STATE") && out[0].contains(GROUP) && out[0].contains("Stable");
+        }, "Expected to find " + GROUP + " in state Stable and the header, but found " + out[0]);
+
+        String[] cgcArgs4 = new String[]{"--bootstrap-server", bootstrapServers(listenerName()), "--list", "--state", "stable"};
+        TestUtils.waitForCondition(() -> {
+            out[0] = kafka.utils.TestUtils.grabConsoleOutput(() -> {
+                ConsumerGroupCommand.main(cgcArgs4);
+                return null;
+            });
+            return out[0].contains("STATE") && out[0].contains(GROUP) && out[0].contains("Stable");
+        }, "Expected to find " + GROUP + " in state Stable and the header, but found " + out[0]);


No, case of parameter differs - "Stable", "stable".

Ah, that's why.

showuon · 2024-01-30T12:40:35Z

+        String[] cgcArgs = new String[]{"--bootstrap-server", bootstrapServers(listenerName()), "--list"};
+        ConsumerGroupCommand.ConsumerGroupService service = getConsumerGroupService(cgcArgs);
+        scala.collection.Set<String> expectedGroups = set(Arrays.asList(GROUP, simpleGroup));
+        final scala.collection.Set[] foundGroups = new scala.collection.Set[1];


Could we use java set? It's weird we compare the 1st element only below. That is, you create a Set, and then, you just verify the 1st element of the Set. Then, why do you need a set if only 1 element?

nizhikov · 2024-01-30T13:46:19Z

@showuon Thanks for review. Answered to all your comments. Please, take a look one more time.

showuon

LGTM!

showuon · 2024-01-31T06:52:34Z

Re-running the CI build.

nizhikov · 2024-01-31T12:17:30Z

CI results looks good for me.

nizhikov · 2024-02-01T08:20:29Z

Hello @showuon

CI is OK for me. Are you ready to merge this PR or have other questions, comments?

showuon · 2024-02-01T08:27:45Z

@nizhikov , thanks for the reminder, I forgot about this PR.
I just checked https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15256/8 and https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15256/7, the test: kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize() failed on each job once. But it didn't appear in earlier run, https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15256/4/, as well as the Kafka trunk build: https://ci-builds.apache.org/job/Kafka/job/kafka/job/trunk/2607/ . Could you help check that? Thanks.

dajac · 2024-02-01T10:10:09Z

@nizhikov I merged #15253. I am not sure if it has any impact on this PR. Does it?

nizhikov · 2024-02-01T10:11:54Z

@dajac It has impact, for sure :)

I reflect your changes in the main PR #14471 and will merge them to this PR in the nearest time.

nizhikov · 2024-02-01T10:23:37Z

@showuon I run DynamicBrokerReconfigurationTest locally and it passed.
Not sure, how those failures can relate to this PR as I change only specific tests classes.

showuon · 2024-02-01T11:09:39Z

@nizhikov , could you rebase with the latest trunk branch and let's see if it fixed the test?

@showuon I run DynamicBrokerReconfigurationTest locally and it passed. Not sure, how those failures can relate to this PR as I change only specific tests classes.

OK, let's wait and see if the latest CI build fixes the test.

nizhikov · 2024-02-01T11:18:57Z

could you rebase with the latest trunk branch and let's see if it fixed the test?

PR currently merged with the latest trunk changes. 74980be

nizhikov · 2024-02-01T14:33:03Z

@showuon CI results are ready.

AFAICS other tests of DynamicBrokerReconfigurationTest failed in the latest trunk:
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka/detail/trunk/2609/tests

…nIntegrationTest`

nizhikov · 2024-02-10T21:24:46Z

Hello @showuon

I found that DeleteTopicTest.scala don't close resources allocated in case of kraft run.
It was hided by removed tests and lead to the tests failures we observed earlier.

I fixed DeleteTopicTest and now it seems that CI is OK.

Take a look one more time, please.

nizhikov · 2024-02-12T16:29:21Z

Hello, @mimaison @jolshan

Can you, please, take a look.
Looks like this PR are ready to merge.

showuon · 2024-02-13T03:01:52Z

Hello @showuon

I found that DeleteTopicTest.scala don't close resources allocated in case of kraft run. It was hided by removed tests and lead to the tests failures we observed earlier.

I fixed DeleteTopicTest and now it seems that CI is OK.

Take a look one more time, please.

Nice find! Thanks for the fix!

nizhikov · 2024-02-13T06:28:11Z

@showuon Thank you for review and merging!

…ache#15256) This PR is part of apache#14471 Is contains some of ConsoleGroupCommand tests rewritten in java. Intention of separate PR is to reduce changes and simplify review. Reviewers: Luke Chen <showuon@gmail.com>

n.izhikov added 4 commits January 24, 2024 19:50

KAFKA-14589 DeleteConsumerGroupsTest rewritten in java

eb511d8

KAFKA-14589 ListConsumerGroupTest rewritten in java

ea24887

KAFKA-14589 DeleteOffsetsConsumerGroupCommandIntegrationTest rewritte…

ff9807e

…n in java

KAFKA-14589 DeleteOffsetsConsumerGroupCommandIntegrationTest rewritte…

429f062

…n in java

nizhikov changed the title ~~KAFKA-14589 [1/3][WIP] Tests of ConsoleGroupCommand rewritten in java~~ KAFKA-14589 [1/3] Tests of ConsoleGroupCommand rewritten in java Jan 24, 2024

showuon self-assigned this Jan 26, 2024

n.izhikov added 2 commits January 27, 2024 00:02

Merge branch 'trunk' into KAFKA-14589_test2

3d327fd

KAFKA-14589 Merge fixes

d66064e

nizhikov mentioned this pull request Jan 29, 2024

KAFKA-16189; Extend admin to support ConsumerGroupDescribe API #15253

Merged

3 tasks

n.izhikov added 2 commits January 29, 2024 18:52

Merge branch 'trunk' into KAFKA-14589_test2

caabc7c

KAFKA-14589 Reflection changes from apache#15211

0503189

showuon reviewed Jan 30, 2024

View reviewed changes

n.izhikov added 2 commits January 30, 2024 16:21

KAFKA-14589 Code review fixes

5701907

KAFKA-14589 Code review fixes

8b6fb19

showuon approved these changes Jan 31, 2024

View reviewed changes

Merge branch 'trunk' into KAFKA-14589_test2

74980be

n.izhikov added 2 commits February 1, 2024 13:20

KAFKA-14589 Reflecting changes of 6c09cc9

b7b79b8

KAFKA-14589 Reflecting changes of 6c09cc9

33fbe4c

n.izhikov added 20 commits February 5, 2024 14:34

Merge branch 'trunk' into KAFKA-14589_test2

117f7e5

Merge branch 'trunk' into KAFKA-14589_test2

a96865a

Merge branch 'trunk' into KAFKA-14589_test2

836cec8

Merge branch 'trunk' into KAFKA-14589_test2

d1b6b61

Merge branch 'trunk' into KAFKA-14589_test2

5dc358e

KAFKA-14589 Revert test removal to check if it fix `kafka.api.SslAdmi…

84326af

…nIntegrationTest`

KAFKA-14589 Remove test one move time.

ad7ba03

KAFKA-14589 Debug messages added

4da11b9

KAFKA-14589 REMOVE NOT FAILED TEST

80a1102

KAFKA-14589 REMOVE FAILED TEST !!!

295935d

KAFKA-14589 REMOVE NOT FAILED TEST

d80e464

KAFKA-14589 REMOVE NOT FAILED TEST

364da83

KAFKA-14589 ONE OF THAT TEST FAIL

42f5eda

KAFKA-14589 REMOVE NOT FAILED TEST

66b6804

KAFKA-14589 REMOVE NOT FAILED TEST

bb89c6f

KAFKA-14589 REMOVE NOT FAILED TEST

abef0bd

KAFKA-14589 REMOVE NOT FAILED TEST

830b18b

KAFKA-14589 REMOVE NOT FAILED TEST

b3939ae

KAFKA-14589 Fix threads shutdown

63101ff

KAFKA-14589 Fix threads shutdown

bc1a07e

Merge branch 'trunk' into KAFKA-14589_test2

c33131c

showuon merged commit 88c5543 into apache:trunk Feb 13, 2024

showuon mentioned this pull request Feb 13, 2024

KAFKA-15421: fix network thread leak in testThreadPoolResize #14320

Merged

3 tasks

+                      };
+                  }
+                  @Test

Conversation

nizhikov commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

showuon commented Jan 26, 2024

Uh oh!

showuon 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!

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 Jan 30, 2024 • 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!

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 Jan 30, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon Jan 30, 2024 • 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!

nizhikov commented Jan 30, 2024

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented Jan 31, 2024

Uh oh!

nizhikov commented Jan 31, 2024

Uh oh!

nizhikov commented Feb 1, 2024

Uh oh!

showuon commented Feb 1, 2024

Uh oh!

dajac commented Feb 1, 2024

Uh oh!

nizhikov commented Feb 1, 2024

Uh oh!

nizhikov commented Feb 1, 2024

nizhikov commented Jan 24, 2024 •

edited

Loading

nizhikov Jan 30, 2024 •

edited

Loading

nizhikov Jan 30, 2024 •

edited

Loading

showuon Jan 30, 2024 •

edited

Loading