RATIS-1436.Add ratis-shell GroupInfo command #540

codings-dan · 2021-11-23T13:05:51Z

What changes were proposed in this pull request?

Add a sub command to show the group information of a ratis server

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-1436

How was this patch tested?

[~/local/myproject/ratis] [group *] -> %  mvn clean install  assembly:single -DskipTests -Dmaven.javadoc.skip=true -Dlicense.skip=true  -Dfindbugs.skip=true -Denforcer.skip=true -Dgpg.skip=true   -Djsse.enableSNIExtension=false  -Prelease -Papache-release
[~/local/myproject/ratis] [group *]-> % cd ratis-assembly/target
[~/local/myproject/ratis/ratis-assembly/target] [group *] -> % tar -xzf apache-ratis-2.3.0-SNAPSHOT-ratis-shell.tar.gz
[~/local/myproject/ratis/ratis-assembly/target] [group *] -> % cd apache-ratis-2.3.0-SNAPSHOT
[~/local/myproject/ratis/ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT] [group *] -> % bin/ratis sh groupInfo -peers localhost:19200,localhost:19201,localhost:19202 -serverAddress localhost:19200
The server localhost:19200 is in 1 groups, and the groupIds is: [group-ABB3109A44C1]

codings-dan · 2021-11-23T13:06:35Z

@maobaolong Can you help me review the code, thx!

szetszwo · 2021-11-24T06:10:26Z

How about add a new "info" option to the existing group command?

codings-dan · 2021-11-24T06:51:18Z

@szetszwo I think the objects targeted by these two commands are different. The group command operates on peers, and the operation in this pr is groups. I want to add sub option to delete and add groups to specific peers in this command. Therefore, can we consider changing the name of the group command to a name related to Peer. Thx!

szetszwo · 2021-11-24T08:10:38Z

You are right. The existing GroupCommand should be renamed to PeerCommand. The new command in this change should be called GroupCommand. The GroupCommand should correspond to GroupManagementApi. Thanks.

szetszwo

@codings-dan , Thanks for the update.

This is actually a "list" command instead of "info". We should combine this with the InfoCommand (rename it go "group"). Then, use the subcommands

"group -info";
"group -list".

Later on, we may add

"group -add";
"group -remove".

These are all 4 methods in GroupManagementApi.

szetszwo · 2021-11-25T01:33:59Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupInfoCommand.java

+/**
+ * Command for querying the group information of a ratis server.
+ */
+public class GroupInfoCommand extends AbstractRatisCommand{


Please use GroupCommand.

szetszwo · 2021-11-25T01:35:14Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupInfoCommand.java

+
+  @Override
+  public String getCommandName() {
+    return "groupInfo";


Please use "group" for the main command and "group -list" for the sub command.

codings-dan · 2021-12-02T15:10:25Z

@szetszwo Sorry, due to a lot of things this week, this patch update is late. The code has been updated, I have tested the function, and can be used normally. Can you help review the code, thx!

szetszwo

@codings-dan thanks for the update. A few comments inlined; see also https://issues.apache.org/jira/secure/attachment/13036937/540_review.patch

BTW, please include the JIRA number in the title of this pull request.

szetszwo · 2021-12-03T02:22:46Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupCommand.java

+  private static final Map<String, Function<Context, ? extends Command>>
+          SUB_COMMANDS = new HashMap<>();
+
+  static {
+    SUB_COMMANDS.put("info", GroupInfoCommand::new);
+    SUB_COMMANDS.put("list", GroupListCommand::new);
+  }
+
+  private Map<String, Command> mSubCommands = new HashMap<>();

  /**
   * @param context command context
   */
  public GroupCommand(Context context) {
    super(context);
+    SUB_COMMANDS.forEach((name, constructor) -> {
+      mSubCommands.put(name, constructor.apply(context));
+    });
  }


This can be simplified as below:

private static final List<Function<Context, AbstractRatisCommand>> SUB_COMMAND_CONSTRUCTORS = Collections.unmodifiableList(Arrays.asList( GroupInfoCommand::new, GroupListCommand::new)); private final Map<String, Command> subs; /** * @param context command context */ public GroupCommand(Context context) { super(context); this.subs = Collections.unmodifiableMap(SUB_COMMAND_CONSTRUCTORS.stream() .map(constructor -> constructor.apply(context)) .collect(Collectors.toMap(Command::getCommandName, Function.identity()))); }

szetszwo · 2021-12-03T02:24:51Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupCommand.java

-            getCommandName(), PEER_OPTION_NAME, GROUPID_OPTION_NAME,
-            REMOVE_OPTION_NAME, ADD_OPTION_NAME);
+  public boolean hasSubCommand() {
+    return true;


Let's change the default to below. Then, we don't have to override it anymore.

return Optional.ofNullable(getSubCommands()).filter(subs -> !subs.isEmpty()).isPresent();

szetszwo · 2021-12-03T02:29:06Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/group/GroupListCommand.java

+    String[] str = strAddr.split(":");
+    if(str.length < 2) {
+      throw new IllegalArgumentException("Failed to parse the server address parameter \"" + strAddr + "\".");
+    }
+    final InetSocketAddress serverAddress = InetSocketAddress.createUnresolved(str[0], Integer.parseInt(str[1]));


Let's add a utility method so that it can be used here and also in other commands.

public static InetSocketAddress parseInetSocketAddress(String address) { try { final String[] hostPortPair = address.split(":"); if (hostPortPair.length < 2) { throw new IllegalArgumentException("Unexpected address format <HOST:PORT>."); } return new InetSocketAddress(hostPortPair[0], Integer.parseInt(hostPortPair[1])); } catch (Exception e) { throw new IllegalArgumentException("Failed to parse the server address parameter \"" + address + "\".", e); } }

szetszwo · 2021-12-03T02:29:59Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupCommand.java

  }

  /**
   * @return command's description
   */
  public static String description() {
-    return "Remove or Add peers of a ratis group";
+    return "Manage the ratis gropu, See sub-commands' descriptions for more details.";


This is a typo. Let's change the message to

return "Manage ratis groups; see the sub-commands for the details.";

codings-dan · 2021-12-03T04:03:15Z

@szetszwo Thank you for reviewing the code, I updated the code according to the comment and patch you left, please see if it needs to be updated again, thx!

szetszwo

This is a typo in GroupCommand.description(); see the comment inlined. The change look good other than that.

codings-dan · 2021-12-03T07:18:41Z

I have fixed the typo, PTAL, thx!

szetszwo · 2021-12-03T08:16:06Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/GroupCommand.java

+  public boolean hasSubCommand() {
+    return Optional.ofNullable(getSubCommands()).filter(sub -> !subs.isEmpty()).isPresent();
  }


@codings-dan , We don't have to override hasSubCommand() anymore since the default will always work. Please remove this. Sorry that I did not see this last time.

I have delete the code, PTAL again, thx!

szetszwo

+1 the change looks good.

Add ratis-shell GroupInfo command

d56dcbb

rename command name

0762d00

szetszwo requested changes Nov 25, 2021

View reviewed changes

codings-dan added 2 commits December 2, 2021 23:05

make code better

a80b6e5

fix checkstyle error

20e26bf

szetszwo reviewed Dec 3, 2021

View reviewed changes

codings-dan changed the title ~~Add ratis-shell GroupInfo command~~ RATIS-1436.Add ratis-shell GroupInfo command Dec 3, 2021

make code better

530d4e7

make code better

fbafce3

szetszwo reviewed Dec 3, 2021

View reviewed changes

make code better

cc8e7f3

szetszwo reviewed Dec 3, 2021

View reviewed changes

make code better

404b830

szetszwo approved these changes Dec 3, 2021

View reviewed changes

szetszwo merged commit 1f4ec88 into apache:master Dec 3, 2021

codings-dan deleted the groupinfo branch January 21, 2022 03:46

symious pushed a commit to symious/ratis that referenced this pull request Feb 26, 2024

RATIS-1436.Add ratis-shell GroupInfo command (apache#540)

effc9a1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RATIS-1436.Add ratis-shell GroupInfo command #540

RATIS-1436.Add ratis-shell GroupInfo command #540

codings-dan commented Nov 23, 2021

codings-dan commented Nov 23, 2021

szetszwo commented Nov 24, 2021

codings-dan commented Nov 24, 2021

szetszwo commented Nov 24, 2021

szetszwo left a comment

szetszwo Nov 25, 2021

szetszwo Nov 25, 2021

codings-dan commented Dec 2, 2021

szetszwo left a comment

szetszwo Dec 3, 2021

szetszwo Dec 3, 2021

szetszwo Dec 3, 2021

szetszwo Dec 3, 2021

codings-dan commented Dec 3, 2021

szetszwo left a comment

codings-dan commented Dec 3, 2021

szetszwo Dec 3, 2021

codings-dan Dec 3, 2021

szetszwo left a comment

RATIS-1436.Add ratis-shell GroupInfo command #540

RATIS-1436.Add ratis-shell GroupInfo command #540

Conversation

codings-dan commented Nov 23, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

codings-dan commented Nov 23, 2021

szetszwo commented Nov 24, 2021

codings-dan commented Nov 24, 2021

szetszwo commented Nov 24, 2021

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codings-dan commented Dec 2, 2021

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codings-dan commented Dec 3, 2021

szetszwo left a comment

Choose a reason for hiding this comment

codings-dan commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment