[#80][Part-1] feat: Add decommisson logic to shuffle server #606

xianjingfeng · 2023-02-16T03:08:21Z

What changes were proposed in this pull request?

Add decommisson logic to shuffle server

Why are the changes needed?

Support shuffle server decommission. It is a part of #80

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java

codecov-commenter · 2023-02-16T03:26:59Z

Codecov Report

Merging #606 (470cd8c) into master (ffa26b4) will increase coverage by 1.96%.
The diff coverage is 63.29%.

@@             Coverage Diff              @@
##             master     #606      +/-   ##
============================================
+ Coverage     60.90%   62.87%   +1.96%     
+ Complexity     1799     1798       -1     
============================================
  Files           214      202      -12     
  Lines         12381    10489    -1892     
  Branches       1042     1051       +9     
============================================
- Hits           7541     6595     -946     
+ Misses         4437     3547     -890     
+ Partials        403      347      -56

Impacted Files	Coverage Δ
...n/java/org/apache/uniffle/common/ServerStatus.java	`0.00% <0.00%> (ø)`
...ffle/common/exception/InvalidRequestException.java	`0.00% <0.00%> (ø)`
.../java/org/apache/uniffle/server/ShuffleServer.java	`61.69% <70.90%> (-2.70%)`	⬇️
...a/org/apache/uniffle/server/ShuffleServerConf.java	`99.33% <100.00%> (+0.02%)`	⬆️
...a/org/apache/uniffle/server/RegisterHeartBeat.java	`43.85% <0.00%> (-43.86%)`	⬇️
...he/uniffle/server/buffer/ShuffleBufferManager.java	`82.74% <0.00%> (-0.36%)`	⬇️
...rnetes/operator/pkg/webhook/inspector/inspector.go
...tor/pkg/controller/sync/coordinator/coordinator.go
...oy/kubernetes/operator/pkg/utils/shufflerserver.go
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

common/src/main/java/org/apache/uniffle/common/ServerStatus.java

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

advancedxy · 2023-02-16T08:35:36Z

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

+  }
+
+  public void decommission() {
+    checkStatusForDecommission();


I believe decommission should be idempotent?

Once the shuffle server is entering decomissioning state, following calls should just do nothing, rather than throw an exception?

There will be other statuses in the future, such as Upgrading.

it doesn't matter that much?

Upgrading could also been decommissoned?

If current status is Upgrading, and than invoke decommisson, the status will be changed to decommissoning?

what do you in mind the Upgrading status mean? when the shuffle server would be upgrading?

I mean if the server is not decomissioning and not in normal status(such as upgrading), decommisson command should be reject. How about if server is decomissioning, we do nothing, but if server is upgrading, throw an exception?

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

server/src/test/java/org/apache/uniffle/server/ShuffleServerTest.java

kaijchen

Thanks @xianjingfeng for the work, some suggestions here.

server/src/test/java/org/apache/uniffle/server/ShuffleServerTest.java

common/src/main/java/org/apache/uniffle/common/ServerStatus.java

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

advancedxy

I believe this pr is in good shape, and almost ready to go. Left some minor comments.

docs/server_guide.md

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

server/src/test/java/org/apache/uniffle/server/ShuffleServerTest.java

kaijchen · 2023-02-17T15:52:33Z

Will it better to use a ExecutorService and use future.cancel() and ExecutorService.shutdownNow() for termination?

Hi @xianjingfeng, sorry for not expressing it correctly. ~~Maybe ForkJoinPool.commonPool() is more appropriate here.~~

Edit: maybe just use a Future for cancel the task.

diff --git a/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java b/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java
index ee8f0775..0c5ecd46 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java
@@ -88,7 +87,7 @@ public class ShuffleServer {
   private MetricReporter metricReporter;
   private volatile ServerStatus serverStatus = ServerStatus.ACTIVE;
   private volatile boolean running;
   private ExecutorService executorService;
+  private Future<?> decommissionFuture;

   public ShuffleServer(ShuffleServerConf shuffleServerConf) throws Exception {
     this.shuffleServerConf = shuffleServerConf;
@@ -287,11 +286,7 @@ public class ShuffleServer {
     }
     serverStatus = ServerStatus.DECOMMISSIONING;
     LOG.info("Shuffle Server is decommissioning.");
     if (executorService == null) {
       executorService = Executors.newSingleThreadExecutor(
           ThreadUtils.getThreadFactory("shuffle-server-decommission-%d"));
     }
-    executorService.submit(this::waitDecommissionFinish);
+    decommissionFuture = executorService.submit(this::waitDecommissionFinish);
   }

   private void waitDecommissionFinish() {
@@ -332,8 +327,12 @@ public class ShuffleServer {
       LOG.info("Shuffle server is not decommissioning. Nothing needs to be done.");
       return;
     }
-    serverStatus = ServerStatus.ACTIVE;
-    LOG.info("Decommission canceled.");
+    if (decommissionFuture.cancel(true)) {
+      serverStatus = ServerStatus.ACTIVE;
+      LOG.info("Decommission canceled.");
+    } else {
+      LOG.warn("Failed to cancel decommission.");
+    }
   }

   public String getIp() {

xianjingfeng · 2023-02-18T04:49:43Z

Maybe ForkJoinPool.commonPool() is more appropriate here.

What are the benefits？

kaijchen · 2023-02-18T05:44:58Z

What are the benefits？

Keeping a thread pool (singleThreadExecutor) for an uncommon task sounds inefficient.

Edit: it is not a good practice to blocking the thread using ForkJoinPool.commonPool().
But you may consider using Future to cancel the task.

advancedxy · 2023-02-19T12:49:37Z

Edit: maybe just use a Future for cancel the task.

+1 for this.

xianjingfeng · 2023-02-20T02:23:47Z

What are the benefits？

Keeping a thread pool (singleThreadExecutor) for an uncommon task sounds inefficient.

Edit: it is not a good practice to blocking the thread using ForkJoinPool.commonPool(). But you may consider using Future to cancel the task.

Done

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

advancedxy · 2023-02-20T10:59:22Z

@xianjingfeng Hi, could you rebase you code with the latest master?
The CI is broken which has been addressed in #631

kaijchen

Hi @xianjingfeng, thanks for updating the PR and docs. Some comments regarding to the "cancel decommission".

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

kaijchen · 2023-02-20T11:39:54Z

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

+    if (decommissionFuture.cancel(true)) {
+      LOG.info("Decommission canceled.");
+    } else {
+      LOG.warn("Failed to cancel decommission.");
+    }


The result of decommissionFuture.cancel() doesn't matter now. Since "cancel" always brings a server back to ACTIVE. Maybe we should call executorService.shutdownNow() here, @advancedxy WDYT?

Suggested change

if (decommissionFuture.cancel(true)) {

LOG.info("Decommission canceled.");

} else {

LOG.warn("Failed to cancel decommission.");

}

if (decommissionFuture != null) {

decommissionFuture.cancel(true);

}

No. Cancel is always need. And we cannot call shutdownNow here, otherwise we cannot decommission again.

advancedxy

Approving this since no blocking issues.

Co-authored-by: Kaijie Chen <ckj@apache.org>

jerqi

LGTM

kaijchen · 2023-02-21T07:39:21Z

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

@@ -149,6 +163,7 @@ public void stopServer() throws Exception {
    }
    SecurityContextFactory.get().getSecurityContext().close();
    server.stop();
+    running = false;


Shutdown the executorService here?

if (executorService != null) { executorService.shutdown() }

kaijchen

LGTM, thanks @xianjingfeng.

xianjingfeng · 2023-02-21T14:07:20Z

Thanks @kaijchen @advancedxy @jerqi for the review

…ache#606) ### What changes were proposed in this pull request? Add decommisson logic to shuffle server ### Why are the changes needed? Support shuffle server decommission. It is a part of apache#80 ### Does this PR introduce any user-facing change? No ### How was this patch tested? UT

xianjingfeng added 14 commits February 2, 2023 18:06

Temporary submission

21f1e17

Merge branch 'master' into issue_80_2

cdf91fc

Temporary submission

6ef0ba6

Temporary submission

5663623

Merge branch 'master' into issue_80_2

6b62ffc

Complete the rpc section

8559119

Optimize

6d433ae

Merge branch 'master' into issue_80_2

376320f

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java

Fix code style.

5e33f33

Optimize

4d1f539

Optimize

ffa3456

Optimize

7b37fa7

Merge branch 'issue_80_2' into issue_80_part1

7527971

Add UT

a58922c

xianjingfeng requested review from zuston, jerqi and kaijchen and removed request for zuston and jerqi February 16, 2023 03:08

xianjingfeng assigned advancedxy and unassigned advancedxy Feb 16, 2023

xianjingfeng requested a review from advancedxy February 16, 2023 03:08

xianjingfeng mentioned this pull request Feb 16, 2023

[#80] feat: Support shuffle server decommission #595

Closed

xianjingfeng added 2 commits February 16, 2023 11:17

Remove refs of protobuf

611551d

Remove ServerStatusTest

f42c756

advancedxy reviewed Feb 16, 2023

View reviewed changes

kaijchen reviewed Feb 16, 2023

View reviewed changes

advancedxy reviewed Feb 17, 2023

View reviewed changes

kaijchen reviewed Feb 17, 2023

View reviewed changes

server/src/test/java/org/apache/uniffle/server/ShuffleServerTest.java Outdated Show resolved Hide resolved

xianjingfeng added 3 commits February 17, 2023 23:12

Optimize

a74bc7d

Optimize

26daa60

remove empty newline

ab7af55

Optimize

301f007

advancedxy reviewed Feb 20, 2023

View reviewed changes

server/src/main/java/org/apache/uniffle/server/ShuffleServer.java Show resolved Hide resolved

Optimize

cb334f0

Merge branch 'master' into issue_80_part1

3c63cc3

kaijchen reviewed Feb 20, 2023

View reviewed changes

advancedxy previously approved these changes Feb 20, 2023

View reviewed changes

Update server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

470cd8c

Co-authored-by: Kaijie Chen <ckj@apache.org>

xianjingfeng dismissed advancedxy’s stale review via 470cd8c February 21, 2023 02:17

advancedxy previously approved these changes Feb 21, 2023

View reviewed changes

jerqi previously approved these changes Feb 21, 2023

View reviewed changes

kaijchen reviewed Feb 21, 2023

View reviewed changes

Optimize

6e1ef4a

xianjingfeng dismissed stale reviews from jerqi and advancedxy via 6e1ef4a February 21, 2023 12:50

kaijchen approved these changes Feb 21, 2023

View reviewed changes

xianjingfeng merged commit 711f089 into apache:master Feb 21, 2023

xianjingfeng deleted the issue_80_part1 branch February 21, 2023 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#80][Part-1] feat: Add decommisson logic to shuffle server #606

[#80][Part-1] feat: Add decommisson logic to shuffle server #606

xianjingfeng commented Feb 16, 2023

codecov-commenter commented Feb 16, 2023 •

edited

Loading

advancedxy Feb 16, 2023

xianjingfeng Feb 16, 2023

advancedxy Feb 16, 2023

xianjingfeng Feb 16, 2023

advancedxy Feb 16, 2023

xianjingfeng Feb 17, 2023

xianjingfeng Feb 17, 2023

kaijchen left a comment

advancedxy left a comment

kaijchen commented Feb 17, 2023 •

edited

Loading

xianjingfeng commented Feb 18, 2023

kaijchen commented Feb 18, 2023 •

edited

Loading

advancedxy commented Feb 19, 2023

xianjingfeng commented Feb 20, 2023

advancedxy commented Feb 20, 2023

kaijchen left a comment

kaijchen Feb 20, 2023

advancedxy Feb 20, 2023

advancedxy left a comment

jerqi left a comment

kaijchen Feb 21, 2023

xianjingfeng Feb 21, 2023

kaijchen left a comment

xianjingfeng commented Feb 21, 2023

[#80][Part-1] feat: Add decommisson logic to shuffle server #606

[#80][Part-1] feat: Add decommisson logic to shuffle server #606

Conversation

xianjingfeng commented Feb 16, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Feb 16, 2023 • edited Loading

Codecov Report

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

kaijchen left a comment

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

kaijchen commented Feb 17, 2023 • edited Loading

xianjingfeng commented Feb 18, 2023

kaijchen commented Feb 18, 2023 • edited Loading

advancedxy commented Feb 19, 2023

xianjingfeng commented Feb 20, 2023

advancedxy commented Feb 20, 2023

kaijchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaijchen left a comment

Choose a reason for hiding this comment

xianjingfeng commented Feb 21, 2023

codecov-commenter commented Feb 16, 2023 •

edited

Loading

kaijchen commented Feb 17, 2023 •

edited

Loading

kaijchen commented Feb 18, 2023 •

edited

Loading