Add decommission capability and removenode cmd #18530

lucyge2022 · 2024-02-26T22:15:48Z

What changes are proposed in this pull request?

Add decommission capability for worker and removenode cli cmd to remove node from etcd registration

Why are the changes needed?

to add decommission capability for worker

Does this PR introduce any user facing changes?

New cli added for remove worker node

lucyge2022 · 2024-02-27T00:52:45Z

addresses #18495 (comment)
FYI @jja725

dbw9580 · 2024-02-27T06:19:28Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

+    Optional<WorkerInfo> targetWorker = getAllMembers().getWorkerById(worker.getIdentity());
+    if (!targetWorker.isPresent()) {
+      throw new InvalidArgumentException(
+          String.format("Unrecognized or non-existing worker:%s", worker.getIdentity()));


Suggested change

String.format("Unrecognized or non-existing worker:%s", worker.getIdentity()));

String.format("Unrecognized or non-existing worker: %s", worker.getIdentity()));

dbw9580 · 2024-02-27T06:19:40Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

+    // Worker should already be offline
+    if (targetWorker.get().getState() != WorkerState.LOST) {
+      throw new InvalidArgumentException(
+          String.format("Can't decommission running worker:%s, stop the worker"


Suggested change

String.format("Can't decommission running worker:%s, stop the worker"

String.format("Can't decommission running worker: %s, stop the worker"

dbw9580 · 2024-02-27T06:27:09Z

dora/core/common/src/main/java/alluxio/wire/WorkerIdentity.java

+   * @return WorkerIdentity object
+   * @throws InvalidArgumentException
+   */
+  public static WorkerIdentity fromString(String workerIdentityStr)


Move this method close to toString so that hopefully people will remember to keep the two implementations compatible when they change either of them in the future. Also please add a unit test to ensure the string representation generated by toString can be correctly parsed by fromString.

done and done

dbw9580 · 2024-02-27T06:36:10Z

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

+
+  @Override
+  public String getUsage() {
+    return "remove";


usage string looks like getCommandName() + " -n <worker_id> | -h".

thanks for catching, done

dbw9580 · 2024-02-27T06:38:58Z

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

+
+  @Override
+  public String getDescription() {
+    return "Remove given worker from the cluster";


Suggested change

return "Remove given worker from the cluster";

return "Remove a worker from the cluster, so that clients and other workers will not consider the removed worker for services. The worker must have been stopped before it can be safely removed from the cluster.";

dbw9580 · 2024-02-27T06:39:32Z

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

+    membershipManager.decommission(
+        new WorkerInfo().setIdentity(WorkerIdentity.fromString(workerId)));
+    mPrintStream.println(String.format(
+        "Successfully decommissioned worker:%s", workerId));


Suggested change

"Successfully decommissioned worker:%s", workerId));

"Successfully decommissioned worker: %s", workerId));

dbw9580 · 2024-02-27T06:44:34Z

dora/core/common/src/main/java/alluxio/wire/WorkerIdentity.java

+    String prefix = "worker-";
+    String errStr = "Unrecognized worker identity string.";
+    if (!workerIdentityStr.startsWith(prefix)) {
+      throw new InvalidArgumentException(errStr);
+    }


I don't think missing the worker- prefix warrants an error, i.e. both worker-1 and 1 should be considered valid inputs, and the parser should be tolerant of the optional prefix.

i actually wanted to make sure tostring and fromstring are strictly paired, i've also added the warning comment should someone wants to change it.

dbw9580 · 2024-02-27T06:51:02Z

dora/core/common/src/main/java/alluxio/wire/WorkerIdentity.java

+    try {
+      return ParserV0.INSTANCE.fromLong(Long.parseLong(idStr));
+    } catch (NumberFormatException ex) {
+      throw new InvalidArgumentException(errStr);


Echo user input:

Suggested change

throw new InvalidArgumentException(errStr);

throw new InvalidArgumentException(errStr + ": " + workerIdentityStr);

elega · 2024-02-27T07:30:01Z

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

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


Should we also make changes to nodes.go?

yes Rico is making the change

added now with the running format:
bin/alluxio process remove-worker -n <worker_id>

Xenorith · 2024-02-27T20:34:49Z

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

+    membershipManager.decommission(
+        new WorkerInfo().setIdentity(WorkerIdentity.fromString(workerId)));
+    mPrintStream.println(String.format(
+        "Successfully decommissioned worker: %s", workerId));


Suggested change

"Successfully decommissioned worker: %s", workerId));

"Successfully removed worker from membership: %s", workerId));

i'd avoid using the word "decommission" from the user facing side as much as possible to avoid confusion

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java

dbw9580

LGTM

Xenorith

will you add the golang cli code as part of this PR?

lucyge2022 · 2024-02-28T18:21:30Z

will you add the golang cli code as part of this PR?

just realized i haven't pushed yesterday, i just pushed the changes, can u take a quick look ?

Xenorith · 2024-02-28T19:15:17Z

cli/src/alluxio.org/cli/cmd/process/remove_worker.go

@@ -0,0 +1,46 @@
+package process


missing license header in new file

…rCommand.java Co-authored-by: Bowen Ding <[email protected]>

lucyge2022 · 2024-02-29T01:00:55Z

alluxio-bot, merge this please.

add decommission capability and removenode cmd

ad1a709

lucyge2022 requested a review from dbw9580 February 26, 2024 22:15

spotbugs fix

0514fd8

dbw9580 suggested changes Feb 27, 2024

View reviewed changes

elega reviewed Feb 27, 2024

View reviewed changes

lucyge2022 added 2 commits February 27, 2024 12:18

address review comments

b305f18

further cleanup

c361a00

Xenorith reviewed Feb 27, 2024

View reviewed changes

lucyge2022 added 2 commits February 27, 2024 12:58

refine output msg

c81ca8d

add remove-worker cmd in cli

802d02b

dbw9580 reviewed Feb 28, 2024

View reviewed changes

dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorkerCommand.java Outdated Show resolved Hide resolved

dbw9580 approved these changes Feb 28, 2024

View reviewed changes

Xenorith approved these changes Feb 28, 2024

View reviewed changes

lucyge2022 and others added 2 commits February 28, 2024 13:49

add missing license

409814e

Update dora/shell/src/main/java/alluxio/cli/fsadmin/nodes/RemoveWorke…

976ec25

…rCommand.java Co-authored-by: Bowen Ding <[email protected]>

lucyge2022 added the type-feature This issue is a feature request label Feb 29, 2024

alluxio-bot merged commit 1bdd219 into Alluxio:main Feb 29, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decommission capability and removenode cmd #18530

Add decommission capability and removenode cmd #18530

lucyge2022 commented Feb 26, 2024

lucyge2022 commented Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024 •

edited

Loading

lucyge2022 Feb 27, 2024

dbw9580 Feb 27, 2024

lucyge2022 Feb 27, 2024

elega Feb 27, 2024

lucyge2022 Feb 27, 2024

lucyge2022 Feb 27, 2024

Xenorith Feb 27, 2024

lucyge2022 Feb 27, 2024

dbw9580 left a comment

Xenorith left a comment

lucyge2022 commented Feb 28, 2024

Xenorith Feb 28, 2024

lucyge2022 Feb 28, 2024

lucyge2022 commented Feb 29, 2024

	String.format("Unrecognized or non-existing worker:%s", worker.getIdentity()));
	String.format("Unrecognized or non-existing worker: %s", worker.getIdentity()));

	String.format("Can't decommission running worker:%s, stop the worker"
	String.format("Can't decommission running worker: %s, stop the worker"

	return "Remove given worker from the cluster";
	return "Remove a worker from the cluster, so that clients and other workers will not consider the removed worker for services. The worker must have been stopped before it can be safely removed from the cluster.";

	"Successfully decommissioned worker:%s", workerId));
	"Successfully decommissioned worker: %s", workerId));

	throw new InvalidArgumentException(errStr);
	throw new InvalidArgumentException(errStr + ": " + workerIdentityStr);

	"Successfully decommissioned worker: %s", workerId));
	"Successfully removed worker from membership: %s", workerId));

Add decommission capability and removenode cmd #18530

Add decommission capability and removenode cmd #18530

Conversation

lucyge2022 commented Feb 26, 2024

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

lucyge2022 commented Feb 27, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbw9580 Feb 27, 2024 • edited Loading

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

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

dbw9580 left a comment

Choose a reason for hiding this comment

Xenorith left a comment

Choose a reason for hiding this comment

lucyge2022 commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyge2022 commented Feb 29, 2024

dbw9580 Feb 27, 2024 •

edited

Loading