[#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. #2084

yl09099 · 2024-08-23T06:44:17Z

What changes were proposed in this pull request?

At the shuffleId level, data on the local or HDFS needs to be deleted synchronously. In some scenarios, the deletion time needs to be shortened. You can rename folders and delete them asynchronously.

Why are the changes needed?

Fix: #2083

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT.

github-actions · 2024-08-23T07:08:50Z

Test Results

2 966 files ±0 2 966 suites ±0 6h 27m 53s ⏱️ - 1m 6s
1 096 tests ±0 1 094 ✅ ±0 2 💤 ±0 0 ❌ ±0
13 735 runs ±0 13 705 ✅ ±0 30 💤 ±0 0 ❌ ±0

Results for commit 75aaa4f. ± Comparison against base commit bd7c2cc.

♻️ This comment has been updated with latest results.

codecov-commenter · 2024-08-23T08:09:45Z

Codecov Report

Attention: Patch coverage is 11.57895% with 168 lines in your changes missing coverage. Please review.

Project coverage is 52.60%. Comparing base (34bf686) to head (9947af7).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
.../handler/impl/HadoopShuffleAsyncDeleteHandler.java	0.00%	47 Missing ⚠️
...storage/handler/impl/AsynDeletionEventManager.java	0.00%	45 Missing ⚠️
...rage/handler/impl/LocalFileAsyncDeleteHandler.java	0.00%	30 Missing ⚠️
...che/uniffle/storage/handler/AsynDeletionEvent.java	0.00%	18 Missing ⚠️
...uniffle/storage/factory/ShuffleHandlerFactory.java	0.00%	8 Missing ⚠️
...age/request/CreateShuffleDeleteHandlerRequest.java	0.00%	7 Missing ⚠️
...pache/uniffle/server/ShuffleServerGrpcService.java	0.00%	3 Missing ⚠️
.../org/apache/uniffle/server/ShuffleTaskManager.java	60.00%	2 Missing ⚠️
...e/uniffle/server/storage/HadoopStorageManager.java	60.00%	1 Missing and 1 partial ⚠️
...he/uniffle/server/storage/LocalStorageManager.java	66.66%	1 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2084      +/-   ##
============================================
+ Coverage     51.84%   52.60%   +0.75%     
- Complexity     2864     3532     +668     
============================================
  Files           469      534      +65     
  Lines         23879    29317    +5438     
  Branches       1966     2731     +765     
============================================
+ Hits          12380    15421    +3041     
- Misses        10726    12905    +2179     
- Partials        773      991     +218

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zuston

Could you help share which case will need to shorten the deletion time.

yl09099 · 2024-08-26T14:29:37Z

Could you help share which case will need to shorten the deletion time.

During the Stage retry, delete the shuffle data block from the disk or hdfs.

yl09099 · 2024-08-27T06:06:57Z

@zuston Help trigger the error module, I have no local error.

zuston

Thanks for your effort on this feature for the stage retry. One comment is how to enable this by config. I hope this feature could be scoped in the explicility config option and be disable by default.

zuston · 2024-09-02T06:06:50Z

server/src/main/java/org/apache/uniffle/server/storage/HadoopStorageManager.java

+  }
+
+  @Override
+  public void removeResources(PurgeEvent event, boolean isQuick) {


Could we make the softDeletion/isQuick as the internal variable in the PurgeEvent?

zuston · 2024-09-02T06:09:25Z

server/src/main/java/org/apache/uniffle/server/storage/HadoopStorageManager.java

+        deleteHandler.quickDelete(asynchronousDeleteEvent);
+        boolean isSucess = quickNeedDeletePaths.offer(asynchronousDeleteEvent);
+        if (!isSucess) {
+          LOG.warn(


This is abnormal that will make the data leaked. For this case, the metrics should be added for better observability

zuston · 2024-09-02T06:12:57Z

server/src/main/java/org/apache/uniffle/server/storage/HadoopStorageManager.java


  HadoopStorageManager(ShuffleServerConf conf) {
    super(conf);
    hadoopConf = conf.getHadoopConf();
    shuffleServerId = conf.getString(ShuffleServerConf.SHUFFLE_SERVER_ID, "shuffleServerId");
    isStorageAuditLogEnabled = conf.getBoolean(ShuffleServerConf.SERVER_STORAGE_AUDIT_LOG_ENABLED);
+    Runnable clearNeedDeletePathTask =


Why not intergrating this part async deletion into the underlying class like SingleStorageManager for localfile and hadoop storage type to share

All comments are complete.

Got it. Let me take a look again

zuston · 2024-09-10T11:26:15Z

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

@@ -227,7 +227,7 @@ public void registerShuffle(
            taskInfo.refreshLatestStageAttemptNumber(shuffleId, stageAttemptNumber);
            try {
              long start = System.currentTimeMillis();
-              shuffleServer.getShuffleTaskManager().removeShuffleDataSync(appId, shuffleId);
+              shuffleServer.getShuffleTaskManager().softRemoveShuffleDataSync(appId, shuffleId);


I hope this could be enabled by the extra config option

Sorry for my previous comment, I think this deletion could be named as TwoPhaseDeletion, which will include 2 phases

Soft deletion

Hard deletion

And for the original deletion way is the hard deletion, we could extra the abstract class to have a good abstraction

Could you use another concept? Because hard deletion makes me think of rm --force. Maybe you can use rename directly.

zuston · 2024-09-10T11:28:34Z

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

@@ -157,6 +157,9 @@ public class ShuffleServerMetrics {
  public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
      "topN_of_on_hadoop_data_size_for_app";

+  private static final String TOTAL_HADOOP_SOFT_DELETE_FAILED = "total_hadoop_soft_delete_failed";


total_hadoop_two_phases_deletion_failed

zuston · 2024-09-10T11:28:42Z

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

@@ -157,6 +157,9 @@ public class ShuffleServerMetrics {
  public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
      "topN_of_on_hadoop_data_size_for_app";

+  private static final String TOTAL_HADOOP_SOFT_DELETE_FAILED = "total_hadoop_soft_delete_failed";
+  private static final String TOTAL_LOCAL_SOFT_DELETE_FAILED = "total_local_soft_delete_failed";


zuston · 2024-09-10T11:30:41Z

server/src/main/java/org/apache/uniffle/server/storage/LocalStorageManager.java

+      }
+    } else {
+      deleteHandler.delete(deletePaths.toArray(new String[deletePaths.size()]), appId, user);
+    }
    removeAppStorageInfo(event);


Is this will effect the metrics analysis when using the 2 phase deletion?

yl09099 · 2024-10-28T12:57:43Z

Overwrite delete logic.

jerqi · 2024-11-21T03:43:55Z

@zuston Could you help me review this pull request?

zuston · 2024-11-25T02:28:23Z

@zuston Could you help me review this pull request?

Yes, I will review this in the later 3 days.

jerqi · 2024-11-26T07:30:03Z

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

@@ -25,11 +25,19 @@ public abstract class PurgeEvent {
  private String appId;
  private String user;
  private List<Integer> shuffleIds;
+  // Quick Delete or not.
+  private boolean isTwoPhasesDeletion;


TwoPhaseDeletion make me confusing. Could you give a better name? Could you add some comments about renaming and deleting the shuffle files?

jerqi · 2024-11-26T07:30:43Z

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

+import org.apache.uniffle.storage.request.CreateShuffleDeleteHandlerRequest;
+import org.apache.uniffle.storage.util.StorageType;
+
+public class AsynDeletionEventManager {


Could you give some comments?

Could you give some comments?

Your above message has been modified.

jerqi · 2024-11-26T07:32:42Z

...e/src/main/java/org/apache/uniffle/storage/handler/impl/HadoopShuffleAsyncDeleteHandler.java

+import org.apache.uniffle.storage.handler.AsynDeletionEvent;
+import org.apache.uniffle.storage.handler.api.ShuffleDeleteHandler;
+import org.apache.uniffle.storage.util.StorageType;
+


HadoopFilesystem already uses renaming in my view. This seems unnecessary for HDFS.

HadoopFilesystem already uses renaming in my view. This seems unnecessary for HDFS.

OK，Let me delete that.

jerqi requested a review from zuston August 23, 2024 06:45

yl09099 force-pushed the uniffle-2083 branch 2 times, most recently from ecf9e44 to 6052399 Compare August 23, 2024 08:00

yl09099 force-pushed the uniffle-2083 branch 5 times, most recently from 8a37d06 to b264a11 Compare August 25, 2024 15:13

zuston reviewed Aug 26, 2024

View reviewed changes

yl09099 force-pushed the uniffle-2083 branch from b264a11 to 52ef203 Compare August 26, 2024 14:24

yl09099 force-pushed the uniffle-2083 branch 2 times, most recently from b7a438c to ccbe953 Compare August 27, 2024 01:58

yl09099 force-pushed the uniffle-2083 branch 2 times, most recently from 96e5fbe to 8126263 Compare September 1, 2024 13:56

yl09099 requested a review from zuston September 1, 2024 14:24

zuston reviewed Sep 2, 2024

View reviewed changes

yl09099 force-pushed the uniffle-2083 branch 10 times, most recently from 0af2052 to a6e8d64 Compare September 9, 2024 06:26

yl09099 force-pushed the uniffle-2083 branch from a6e8d64 to 9145b24 Compare September 9, 2024 07:52

zuston reviewed Sep 10, 2024

View reviewed changes

yl09099 force-pushed the uniffle-2083 branch 2 times, most recently from bb11961 to 9e9721f Compare September 12, 2024 08:47

yl09099 closed this Oct 28, 2024

yl09099 force-pushed the uniffle-2083 branch from 9e9721f to 85b1f01 Compare October 28, 2024 07:29

yl09099 reopened this Oct 28, 2024

jerqi changed the title ~~[Improvement] Quickly delete local or HDFS data at the shuffleId level.~~ [#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. Oct 28, 2024

jerqi requested a review from zuston October 28, 2024 12:59

yl09099 force-pushed the uniffle-2083 branch 9 times, most recently from b8194df to 93be639 Compare November 4, 2024 05:11

[Improvement] Quickly delete local or HDFS data at the shuffleId level.

9947af7

yl09099 force-pushed the uniffle-2083 branch from 93be639 to 9947af7 Compare November 4, 2024 05:20

jerqi reviewed Nov 26, 2024

View reviewed changes

yl09099 force-pushed the uniffle-2083 branch from 7f17f5e to a49aa84 Compare November 28, 2024 07:34

improvement: Quickly delete local or HDFS data at the shuffleId level..

75aaa4f

yl09099 force-pushed the uniffle-2083 branch from a49aa84 to 75aaa4f Compare November 28, 2024 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. #2084

[#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. #2084

yl09099 commented Aug 23, 2024

github-actions bot commented Aug 23, 2024 •

edited

Loading

codecov-commenter commented Aug 23, 2024 •

edited

Loading

zuston left a comment

yl09099 commented Aug 26, 2024

yl09099 commented Aug 27, 2024

zuston left a comment

zuston Sep 2, 2024

zuston Sep 2, 2024

zuston Sep 2, 2024

yl09099 Sep 9, 2024

zuston Sep 10, 2024

zuston Sep 10, 2024

zuston Sep 10, 2024

jerqi Sep 10, 2024

zuston Sep 10, 2024

zuston Sep 10, 2024

zuston Sep 10, 2024

yl09099 commented Oct 28, 2024

jerqi commented Nov 21, 2024

zuston commented Nov 25, 2024

jerqi Nov 26, 2024

jerqi Nov 26, 2024

yl09099 Nov 28, 2024

jerqi Nov 26, 2024

yl09099 Nov 28, 2024

[#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. #2084

Are you sure you want to change the base?

[#2083] improvement: Quickly delete local or HDFS data at the shuffleId level. #2084

Conversation

yl09099 commented Aug 23, 2024

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?

github-actions bot commented Aug 23, 2024 • edited Loading

Test Results

codecov-commenter commented Aug 23, 2024 • edited Loading

Codecov Report

zuston left a comment

Choose a reason for hiding this comment

yl09099 commented Aug 26, 2024

yl09099 commented Aug 27, 2024

zuston 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

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

yl09099 commented Oct 28, 2024

jerqi commented Nov 21, 2024

zuston commented Nov 25, 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

github-actions bot commented Aug 23, 2024 •

edited

Loading

codecov-commenter commented Aug 23, 2024 •

edited

Loading