KAFKA-14589 ConsumerGroupCommand options and case classes rewritten by nizhikov · Pull Request #14856 · apache/kafka

nizhikov · 2023-11-28T15:48:04Z

This PR is part of #14471
It contains ConsumerGroupCommandOptions and case classes used by ConsumerGroupCommand rewritten in java.

The goal of PR is to reduce main PR size.

Committer Checklist (excluded from commit message)

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

…n java

nizhikov · 2023-11-28T15:48:49Z

Hello @mimaison , @jolshan

This PR is first step to transfer ConsumerGroupCommand from scala to java.
Can you, please, take a look.

nizhikov · 2023-12-05T13:55:06Z

@mimaison Will you be able to take a look?

This is first PR to transfer ConsumerGroupCommand to java.

Co-authored-by: Taras Ledkov <tledkov@apache.org>

tledkov · 2023-12-07T06:27:04Z

+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> Set<T> minus(Set<T> set, T...toRemove) {


What about moving this method to CollectionUtils?

Yes maybe we can move this to Utils?

tledkov

The patch is OK with me.
Checked:

java codestyle, java code patterns for new code;
command line string constants.

…merGroupCommandOptions.java Co-authored-by: Taras Ledkov <tledkov@apache.org>

nizhikov · 2023-12-07T12:23:20Z

Hello, @mimaison @jolshan

Looks like very straightforward PR for me.
Can you, please, review it, also?

So we can start moving ConsumerGroupCommand to tools.

Full patch is #14471

…9_options

nizhikov · 2024-01-08T17:17:23Z

@mimaison can you, please, take a look.

mimaison

Thanks for the PR. I left a few minor comments

mimaison · 2024-01-17T16:27:04Z

+        if (options.has(describeOpt)) {
+            if (!options.has(groupOpt) && !options.has(allGroupsOpt))
+                CommandLineUtils.printUsageAndExit(parser,
+                    "Option $describeOpt takes one of these options: " + Utils.join(allGroupSelectionScopeOpts));


"Option $describeOpt takes -> "Option " + describeOpt + " takes`

mimaison · 2024-01-17T16:29:36Z

+        this.clientId = clientId;
+        this.logEndOffset = logEndOffset;
+    }
+}


Nit: Can we add a newline?

mimaison · 2024-01-17T16:30:33Z

+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> Set<T> minus(Set<T> set, T...toRemove) {


Yes maybe we can move this to Utils?

jolshan · 2024-01-17T18:15:36Z

@@ -0,0 +1,263 @@
+/*


Is the tools/consumergroup a new folder? I wonder if there is a name that is more consistent with the other folders.

Is the tools/consumergroup a new folder?

yes.

I wonder if there is a name that is more consistent with the other folders.

Couldn't come up with the better naming :)
Do you have one in mind?

I wasn't sure if we wanted to use the same convention of the other java consumer group files. But maybe that is confusing. They follow a pattern of coordinator/group. Not sure if we really have reusability amongst coordinator tools though.

I think when we have had two word folders we usually put a dash between them (ie consumer-group)

I think when we have had two word folders we usually put a dash between them (ie consumer-group)

Dash can't be used in java package name.

They follow a pattern of coordinator/group

We can move classes to org.apache.kafka.tools.consumer.group package.
What do you think?

That works for me. Thanks!

package renamed.

jolshan · 2024-01-17T18:18:56Z

Did we want to delete the old files in this PR or a follow up?

nizhikov · 2024-01-17T21:52:51Z

@jolshan

Did we want to delete the old files in this PR or a follow up?

For the previous command we remove old files only when command was merged

Reassign case classes and options moved to java - 1fd58e3
Command moved to java and old classes removed - 76b1b50

So I tend to reuse this strategy for the ConsumerGroupCommand.

nizhikov · 2024-01-18T09:22:35Z

@mimaison CI seems OK. I addressed all your comments. Please, take a look.

mimaison

Thanks for the updates. Let's avoid adding stuff into clients that are only used by tools.

mimaison · 2024-01-18T11:15:39Z

+     * @param <T> Element type.
+     */
+    @SuppressWarnings("unchecked")
+    public static <T> Set<T> minus(Set<T> set, T...toRemove) {


Let's not add methods in the client module that are only used in tools. This can be moved to ToolsUtils.

mimaison · 2024-01-18T11:17:07Z

+     * @param collection The list of items
+     * @return The string representation.
+     */
+    public static <T> String join(Collection<T> collection) {


Do we really need this method? Why can't we call the existing join() method and pass the separator? If you want to keep it, since it's only used in tools let's move it to the tools module.

Method removed

nizhikov · 2024-01-22T09:21:07Z

Hello @mimaison , @jolshan do you have any other comments?

mimaison · 2024-01-22T15:30:57Z

+    public final String group;
+    public final Optional<Node> coordinator;
+    public final Optional<String> topic;
+    public final Optional<Integer> partition;


We can use OptionalInt

mimaison · 2024-01-22T15:31:06Z

+    public final Optional<Node> coordinator;
+    public final Optional<String> topic;
+    public final Optional<Integer> partition;
+    public final Optional<Long> offset;


We can use OptionalLong

jolshan · 2024-01-22T18:23:16Z

+            if (!options.has(groupOpt) && !options.has(allGroupsOpt))
+                CommandLineUtils.printUsageAndExit(parser,
+                    "Option " + resetOffsetsOpt + " takes one of these options: " + join(allGroupSelectionScopeOpts, ", "));
+            CommandLineUtils.checkInvalidArgs(parser, options, resetToOffsetOpt,   minus(allResetOffsetScenarioOpts, resetToOffsetOpt));


hmm -- do we want to line up all the minus arguments here? It adds extra spaces that may be against our style guidelines

Extra spaces removed

jolshan · 2024-01-22T18:28:38Z

Thanks @nizhikov just one more small comment from me.

mimaison

LGTM, thanks

mimaison · 2024-01-22T20:18:08Z

@jolshan Feel free to merge once you're happy with the changes.

jolshan · 2024-01-22T21:17:28Z

Thanks. I will wait for the tests to finish 👍

nizhikov · 2024-01-23T12:26:49Z

Thanks for review and merge!

…pache#14856) This PR is part of apache#14471 It contains ConsumerGroupCommandOptions and case classes used by ConsumerGroupCommand rewritten in java. The goal of PR is to reduce main PR size. Co-authored-by: Taras Ledkov <tledkov@apache.org> Reviewers: Mickael Maison <mickael.maison@gmail.com>, Taras Ledkov <tledkov@apache.org>, Justine Olshan <jolshan@confluent.io>

KAFKA-14589 ConsumerGroupCommand options and case classes rewritten i…

c990b26

…n java

nizhikov added 2 commits November 29, 2023 15:38

Merge branch 'trunk' into KAFKA-14589_options

6a6f44f

Merge branch 'trunk' into KAFKA-14589_options

0a8bc05

tledkov reviewed Dec 6, 2023

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/utils/Utils.java Outdated

nizhikov and others added 2 commits December 6, 2023 22:24

Update clients/src/main/java/org/apache/kafka/common/utils/Utils.java

d62882d

Co-authored-by: Taras Ledkov <tledkov@apache.org>

Merge branch 'trunk' into KAFKA-14589_options

a47363b

tledkov reviewed Dec 7, 2023

View reviewed changes

Comment thread tools/src/main/java/org/apache/kafka/tools/consumergroup/ConsumerGroupCommandOptions.java Outdated

tledkov reviewed Dec 7, 2023

View reviewed changes

tledkov approved these changes Dec 7, 2023

View reviewed changes

Update tools/src/main/java/org/apache/kafka/tools/consumergroup/Consu…

295d45e

…merGroupCommandOptions.java Co-authored-by: Taras Ledkov <tledkov@apache.org>

nizhikov added 6 commits December 12, 2023 11:02

Merge branch 'trunk' into KAFKA-14589_options

5e28416

Merge remote-tracking branch 'my/KAFKA-14589_options' into KAFKA-1458…

a424b4e

…9_options

Merge branch 'trunk' into KAFKA-14589_options

4661d8a

Merge branch 'trunk' into KAFKA-14589_options

5eac0a1

Merge branch 'trunk' into KAFKA-14589_options

56fffb1

Merge branch 'trunk' into KAFKA-14589_options

d019bd1

mimaison reviewed Jan 17, 2024

View reviewed changes

jolshan reviewed Jan 17, 2024

View reviewed changes

nizhikov added 2 commits January 18, 2024 00:42

Merge branch 'trunk' into KAFKA-14589_options

1e445f6

KAFKA-14589 Code review fixes

2e81fa9

KAFKA-14589 Rename partition

70a0a4f

mimaison reviewed Jan 18, 2024

View reviewed changes

KAFKA-14589 Review fixes

54986b5

KAFKA-14589 Review fixes

40e8f6d

mimaison reviewed Jan 22, 2024

View reviewed changes

KAFKA-14589 Review fixes

c957587

jolshan reviewed Jan 22, 2024

View reviewed changes

KAFKA-14589 Review fixes

82058f4

mimaison approved these changes Jan 22, 2024

View reviewed changes

jolshan approved these changes Jan 22, 2024

View reviewed changes

jolshan merged commit ff25c35 into apache:trunk Jan 23, 2024

Conversation

nizhikov commented Nov 28, 2023

Committer Checklist (excluded from commit message)

Uh oh!

nizhikov commented Nov 28, 2023

Uh oh!

nizhikov commented Dec 5, 2023

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!

tledkov left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Dec 7, 2023

Uh oh!

nizhikov commented Jan 8, 2024

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolshan commented Jan 17, 2024

Uh oh!

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

Uh oh!

Uh oh!

nizhikov commented Jan 18, 2024

Uh oh!

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

nizhikov commented Jan 22, 2024

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 commented Jan 17, 2024 •

edited

Loading