Introduce data cleanup mechanism on stage level #249

zuston · 2022-09-28T07:39:27Z

What changes were proposed in this pull request?

Introduce data cleanup mechanism on stage level

Why are the changes needed?

This PR is to optimize the disk capacity. For example

For some spark ML jobs, it will run multiple stages and reserve large unused shuffle data in shuffle-servers.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UTs

codecov-commenter · 2022-09-28T08:06:59Z

Codecov Report

Merging #249 (e24fbbd) into master (fc6d49b) will decrease coverage by 0.94%.
The diff coverage is 55.02%.

@@             Coverage Diff              @@
##             master     #249      +/-   ##
============================================
- Coverage     60.11%   59.17%   -0.95%     
- Complexity     1217     1369     +152     
============================================
  Files           150      166      +16     
  Lines          7602     8974    +1372     
  Branches        718      853     +135     
============================================
+ Hits           4570     5310     +740     
- Misses         2791     3390     +599     
- Partials        241      274      +33

Impacted Files	Coverage Δ
...e/uniffle/client/factory/ShuffleClientFactory.java	`0.00% <0.00%> (ø)`
...pache/uniffle/server/ShuffleServerGrpcService.java	`0.87% <0.00%> (-0.03%)`	⬇️
...he/uniffle/server/storage/MultiStorageManager.java	`37.50% <0.00%> (ø)`
...storage/handler/impl/HdfsShuffleDeleteHandler.java	`0.00% <0.00%> (ø)`
...e/storage/handler/impl/LocalFileDeleteHandler.java	`0.00% <0.00%> (ø)`
...he/uniffle/client/impl/ShuffleWriteClientImpl.java	`21.23% <9.67%> (-1.75%)`	⬇️
.../org/apache/uniffle/server/ShuffleTaskManager.java	`76.80% <78.94%> (-0.16%)`	⬇️
...he/uniffle/server/buffer/ShuffleBufferManager.java	`83.33% <86.36%> (+1.17%)`	⬆️
...va/org/apache/uniffle/server/event/PurgeEvent.java	`88.88% <88.88%> (ø)`
.../java/org/apache/spark/shuffle/RssSparkConfig.java	`96.77% <100.00%> (+0.10%)`	⬆️
... and 23 more

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

zuston · 2022-09-28T10:21:25Z

PTAL @jerqi . The unregister is now scoped with Spark, MR will be supported in the next commits.

jerqi · 2022-09-28T11:25:43Z

PTAL @jerqi . The unregister is now scoped with Spark, MR will be supported in the next commits.

Why do the mr need to clean up the shuffle of stage level? MR don't have multiple stages.

zuston · 2022-09-28T11:35:52Z

PTAL @jerqi . The unregister is now scoped with Spark, MR will be supported in the next commits.

Why do the mr need to clean up the shuffle of stage level? MR don't have multiple stages.

Oh yes. My fault.

server/src/main/java/org/apache/uniffle/server/event/AppPurgeEvent.java

zuston · 2022-09-30T15:03:20Z

Gentle ping @jerqi If u have time, could u help review this? I think I have some time to quick fix at national day.

jerqi · 2022-10-08T02:32:38Z

I will take a look at this pr asap.

jerqi · 2022-10-08T09:01:50Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

+    try {
+      ExecutorService executorService =
+          Executors.newFixedThreadPool(
+              Math.min(10, shuffleServerInfoSet.size()),


Why do we use 10?

Emm. Maybe we should use the Runtime.getRuntime().availableProcessors(). And I have no idea on this

10 is the experienced value. The request is lightweight, I think 10 threads are enough. There is no need to introduce extra config to increase the understanding burden for users.

Em... we would better use configuration option.

OK. I will extract config entry.

storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileDeleteHandler.java

jerqi · 2022-10-08T09:28:49Z

One question?
Why do we add the concept of PurgeEvent? What's the advantage? Could we just add a shuffle level deletion method for StorageManager?

zuston · 2022-10-08T09:38:59Z

One question? Why do we add the concept of PurgeEvent? What's the advantage? Could we just add a shuffle level deletion method for StorageManager?

Two reasons.

Make clearResourceThread clean two kinds of events of stage and app level data
I dont want to introduce extra delete method in StorageManager because most of deletion logic is the same

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

jerqi · 2022-10-09T02:55:28Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

+              Math.min(10, shuffleServerInfoSet.size()),
+              ThreadUtils.getThreadFactory("unregister-shuffle-%d")
+          );
+      List<Future<Void>> futures = executorService.invokeAll(callableList, 10, TimeUnit.SECONDS);


10 seconds? Should it be configuration option?

This is the default timeout, considering the request is lightweight, I think 10s is enough. There is no need to introduce extra config to increase the understanding burden for users.

Sometimes the timeout may not be enough, we should give a default value according to our experience. Usually we can't image all the situations that users use.

...t/spark-common/src/test/java/org/apache/uniffle/test/ShuffleUnregisterWithLocalfileTest.java

storage/src/main/java/org/apache/uniffle/storage/handler/impl/HdfsShuffleDeleteHandler.java

zuston · 2022-10-10T06:32:39Z

Updated.

jerqi · 2022-10-11T03:04:08Z

One question? Why do we add the concept of PurgeEvent? What's the advantage? Could we just add a shuffle level deletion method for StorageManager?

Two reasons.

Make clearResourceThread clean two kinds of events of stage and app level data

I dont want to introduce extra delete method in StorageManager because most of deletion logic is the same

Actually we still add a method removeResourcesByShuffleIds. Let me think twice.

zuston · 2022-10-11T09:27:58Z

Changelog of latest commit

Extract the config entries of unregistering thread pool size and request timeout sec

spark.rss.client.unregister.request.timeout.sec and spark.rss.client.unregister.thread.pool.size

zuston · 2022-10-12T03:38:38Z

Gentle ping @jerqi

jerqi

LGTM, thanks @zuston

zuston · 2022-10-12T08:59:13Z

Thanks for your review @jerqi

### What changes were proposed in this pull request? Followup #249. Cleanup code and unify interfaces. ### Why are the changes needed? Cleanup code and unify interfaces. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI.

zuston requested a review from jerqi September 28, 2022 08:57

jerqi reviewed Sep 28, 2022

View reviewed changes

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

zuston requested a review from jerqi September 29, 2022 01:52

jerqi reviewed Oct 8, 2022

View reviewed changes

storage/src/main/java/org/apache/uniffle/storage/handler/impl/LocalFileDeleteHandler.java Show resolved Hide resolved

zuston requested a review from jerqi October 9, 2022 02:02

jerqi reviewed Oct 9, 2022

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Show resolved Hide resolved

jerqi reviewed Oct 9, 2022

View reviewed changes

...t/spark-common/src/test/java/org/apache/uniffle/test/ShuffleUnregisterWithLocalfileTest.java Outdated Show resolved Hide resolved

jerqi reviewed Oct 9, 2022

View reviewed changes

storage/src/main/java/org/apache/uniffle/storage/handler/impl/HdfsShuffleDeleteHandler.java Show resolved Hide resolved

zuston requested a review from jerqi October 9, 2022 08:49

zuston added 7 commits October 11, 2022 17:45

Introduce data cleanup mechanism on stage level

52a5c2c

Add spark ITs

664346d

Add UTs

76239f3

Update spark patch

44807fc

fix

cb27190

extract config entries

ea79dfa

amend-2

ca32441

zuston force-pushed the unregisterShuffle branch from 78e7790 to ca32441 Compare October 11, 2022 09:47

amend-3

e24fbbd

jerqi approved these changes Oct 12, 2022

View reviewed changes

jerqi merged commit 1ee6820 into apache:master Oct 12, 2022

zuston deleted the unregisterShuffle branch October 12, 2022 08:59

jerqi mentioned this pull request Nov 16, 2022

[Improvement] Cleanup unused shuffle servers after stage completed #328

Closed

3 tasks

kaijchen added a commit to kaijchen/incubator-uniffle that referenced this pull request Feb 7, 2023

[Followup apache#249] Cleanup code

f1374cf

kaijchen mentioned this pull request Feb 7, 2023

[Followup #249] refactor: cleanup code and unify interfaces #558

Merged

jerqi mentioned this pull request Feb 28, 2023

[Umbrella] Release 0.7.0 #611

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce data cleanup mechanism on stage level #249

Introduce data cleanup mechanism on stage level #249

zuston commented Sep 28, 2022

codecov-commenter commented Sep 28, 2022 •

edited

zuston commented Sep 28, 2022

jerqi commented Sep 28, 2022

zuston commented Sep 28, 2022

zuston commented Sep 30, 2022

jerqi commented Oct 8, 2022

jerqi Oct 8, 2022

zuston Oct 8, 2022

zuston Oct 9, 2022

jerqi Oct 11, 2022

zuston Oct 11, 2022

jerqi commented Oct 8, 2022

zuston commented Oct 8, 2022

jerqi Oct 9, 2022

zuston Oct 9, 2022

zuston Oct 9, 2022

jerqi Oct 11, 2022 •

edited

zuston Oct 11, 2022

zuston commented Oct 10, 2022

jerqi commented Oct 11, 2022

zuston commented Oct 11, 2022

zuston commented Oct 12, 2022

jerqi left a comment

zuston commented Oct 12, 2022

Introduce data cleanup mechanism on stage level #249

Introduce data cleanup mechanism on stage level #249

Conversation

zuston commented Sep 28, 2022

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 Sep 28, 2022 • edited

Codecov Report

zuston commented Sep 28, 2022

jerqi commented Sep 28, 2022

zuston commented Sep 28, 2022

zuston commented Sep 30, 2022

jerqi commented Oct 8, 2022

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

jerqi commented Oct 8, 2022

zuston commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuston commented Oct 10, 2022

jerqi commented Oct 11, 2022

zuston commented Oct 11, 2022

zuston commented Oct 12, 2022

jerqi left a comment

Choose a reason for hiding this comment

zuston commented Oct 12, 2022

codecov-commenter commented Sep 28, 2022 •

edited

jerqi Oct 11, 2022 •

edited