RATIS-1431. Add ratis-shell add&remove peer command #534

codings-dan · 2021-11-13T10:02:34Z

What changes were proposed in this pull request?

Add a sub command to remove and add peer.

What is the link to the Apache JIRA

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

How was this patch tested?

[~/local/myproject/ratis] [removePeer *] -> %  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] [removePeer *]-> % cd ratis-assembly/target
[~/local/myproject/ratis/ratis-assembly/target] [removePeer *] -> % tar -xzf apache-ratis-2.3.0-SNAPSHOT-ratis-shell.tar.gz
[~/local/myproject/ratis/ratis-assembly/target] [removePeer *] -> % cd apache-ratis-2.3.0-SNAPSHOT
 [~/local/myproject/ratis/ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT] [removePeer *] -> % bin/ratis sh info -peers localhost:19200,localhost:19201,localhost:19202
group id: 02511d47-d67c-49a3-9011-abb3109a44c1
leader info: localhost_19200(localhost:19200)

[server {
  id: "localhost_19200"
  address: "localhost:19200"
}
commitIndex: 8
, server {
  id: "localhost_19202"
  address: "localhost:19202"
}
commitIndex: 8
, server {
  id: "localhost_19201"
  address: "localhost:19201"
}
commitIndex: 8
]
[~/local/myproject/ratis/ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT] [removePeer *]-> % bin/ratis sh quorumRemove -peers localhost:19200,localhost:19201,localhost:19202 -removePeer localhost:19202
[~/local/myproject/ratis/ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT] [removePeer *]-> % bin/ratis sh info -peers localhost:19200,localhost:19201,localhost:19202
group id: 02511d47-d67c-49a3-9011-abb3109a44c1
leader info: localhost_19200(localhost:19200)

[server {
  id: "localhost_19200"
  address: "localhost:19200"
}
commitIndex: 12
, server {
  id: "localhost_19201"
  address: "localhost:19201"
}
commitIndex: 12
]

maobaolong

Thanks for this contribution, left some minor comments.

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

maobaolong · 2021-11-13T10:28:03Z

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

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


"quorumRemove" -> "remove"

@szetszwo What sub command name do you think is better?

quorumRemove

remove

removePeers

"removePeers" sounds good.

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

codings-dan · 2021-11-15T11:13:41Z

@maobaolong I modified the code according to your suggestion. PTAL, thx!
What do you think about commandName?@szetszwo

szetszwo · 2021-11-17T07:06:45Z

@codings-dan , @maobaolong , I think we should combine addPeers and removePeers to a single setConfiguration command so that users can add and remove peers at the same time. What do you think?

codings-dan · 2021-11-17T11:40:22Z

@szetszwo You mean that the original cluster members were A, B, and C, but now they become A, B, and D, right?

szetszwo · 2021-11-17T11:46:01Z

Yes.

szetszwo · 2021-11-17T11:49:43Z

The command could look like:

ratis sh setConf -groupid xxx -peers a0,a1,a2 -remove a2 -add a3

codings-dan · 2021-11-17T12:02:46Z

I think this is a good idea, but I tend to modify it to
ratis sh memberChange -groupid xxx -peers a0,a1,a2 -remove a2 -add a3
For setConf is not very use-friendly, what do you think? @szetszwo

szetszwo · 2021-11-17T14:41:20Z

How about "group"?

ratis sh group -groupid xxx -peers a0,a1,a2 -remove a2 -add a3

codings-dan · 2021-11-17T15:26:18Z

Ok, I will modify the code according to this comment

codings-dan · 2021-11-18T09:51:31Z

@szetszwo I modified the code according to your comment. PTAL, thx! And I have tested it locally and there is no problem with the function.

szetszwo

@codings-dan , thanks for the update. Some comments inlined and some suggestions can be found in https://issues.apache.org/jira/secure/attachment/13036297/534_review.patch . (It did not rename QuorumCommand to GroupCommand so that it is easily to see the changes. Please do the rename.)

szetszwo · 2021-11-18T15:15:33Z

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

+/**
+ * Command for remove ratis server.
+ */
+public class QuorumCommand extends AbstractRatisCommand {


Let's call it GroupCommand.

szetszwo · 2021-11-18T15:16:25Z

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

+  public static final String REMOVE_PEER_ADDRESS_OPTION_NAME = "removePeer";
+  public static final String ADD_PEER_ADDRESS_OPTION_NAME = "addPeer";


Let's use "remove" and "add".

szetszwo · 2021-11-18T15:58:22Z

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

+  private String[] isFormat(String address) {
+    String[] str = address.split(":");
+    if(str.length < 2) {
+      println("The format of the parameter is wrong");


Let's throw an exception.

maobaolong · 2021-11-18T16:54:43Z

@codings-dan Please address the comments from @szetszwo and update the description of this PR, combine the add and remove into one command sounds a good idea.

codings-dan · 2021-11-19T03:06:37Z

@szetszwo Thank you for reviewing the code, I modified it according to the patch you left, PTAL, thx!

szetszwo

+1 the change looks good.

szetszwo · 2021-11-19T03:11:27Z

@codings-dan thanks for updating the change and this pull request. Could you also update the JIRAs?

codings-dan · 2021-11-19T03:37:11Z

Jira has been updated.

Add ratis-shell removePeer command

466949b

codings-dan changed the title ~~Add ratis-shell removePeer command~~ RATIS-1431. Add ratis-shell removePeer command Nov 13, 2021

maobaolong suggested changes Nov 13, 2021

View reviewed changes

codings-dan added 2 commits November 15, 2021 19:14

Merge branch 'master' of https://github.com/apache/ratis into removePeer

a33e29b

make code better

d913886

codings-dan force-pushed the removePeer branch from 21ddd95 to d913886 Compare November 15, 2021 11:17

codings-dan requested a review from szetszwo November 17, 2021 07:15

make code better

1383d6b

make code better

dfaf4f7

szetszwo requested changes Nov 18, 2021

View reviewed changes

codings-dan changed the title ~~RATIS-1431. Add ratis-shell removePeer command~~ RATIS-1431. Add ratis-shell add and remove peer command Nov 19, 2021

codings-dan changed the title ~~RATIS-1431. Add ratis-shell add and remove peer command~~ RATIS-1431. Add ratis-shell add&remove peer command Nov 19, 2021

make code better

316cb1f

codings-dan requested a review from szetszwo November 19, 2021 03:07

szetszwo approved these changes Nov 19, 2021

View reviewed changes

szetszwo merged commit 2c86489 into apache:master Nov 19, 2021

codings-dan mentioned this pull request Nov 19, 2021

RATIS-1430. Add ratis-shell addPeer command #535

Closed

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

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

RATIS-1431. Add ratis-shell add&remove peer command (apache#534)

d908381

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RATIS-1431. Add ratis-shell add&remove peer command #534

RATIS-1431. Add ratis-shell add&remove peer command #534

codings-dan commented Nov 13, 2021 •

edited

Loading

maobaolong left a comment

maobaolong Nov 13, 2021

maobaolong Nov 13, 2021

szetszwo Nov 17, 2021

codings-dan commented Nov 15, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

szetszwo commented Nov 17, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

codings-dan commented Nov 18, 2021

szetszwo left a comment

szetszwo Nov 18, 2021

szetszwo Nov 18, 2021

szetszwo Nov 18, 2021

maobaolong commented Nov 18, 2021

codings-dan commented Nov 19, 2021

szetszwo left a comment

szetszwo commented Nov 19, 2021

codings-dan commented Nov 19, 2021

		public static final String REMOVE_PEER_ADDRESS_OPTION_NAME = "removePeer";
		public static final String ADD_PEER_ADDRESS_OPTION_NAME = "addPeer";

RATIS-1431. Add ratis-shell add&remove peer command #534

RATIS-1431. Add ratis-shell add&remove peer command #534

Conversation

codings-dan commented Nov 13, 2021 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

maobaolong left a comment

Choose a reason for hiding this comment

maobaolong Nov 13, 2021

Choose a reason for hiding this comment

maobaolong Nov 13, 2021

Choose a reason for hiding this comment

szetszwo Nov 17, 2021

Choose a reason for hiding this comment

codings-dan commented Nov 15, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

szetszwo commented Nov 17, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

szetszwo commented Nov 17, 2021

codings-dan commented Nov 17, 2021

codings-dan commented Nov 18, 2021

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo Nov 18, 2021

Choose a reason for hiding this comment

szetszwo Nov 18, 2021

Choose a reason for hiding this comment

szetszwo Nov 18, 2021

Choose a reason for hiding this comment

maobaolong commented Nov 18, 2021

codings-dan commented Nov 19, 2021

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo commented Nov 19, 2021

codings-dan commented Nov 19, 2021

codings-dan commented Nov 13, 2021 •

edited

Loading