[CELEBORN-764] Fix celeborn on HDFS might clean using app directories. #1678

FMX · 2023-07-04T14:27:53Z

What changes were proposed in this pull request?

Make Celeborn leader clean expired app dirs on HDFS when an application is Lost.

Why are the changes needed?

If Celeborn is working on HDFS, the storage manager starts and cleans expired app directories, and the newly created worker will want to delete any unknown app directories.
This will cause using app directories to be deleted unexpectedly.

Does this PR introduce any user-facing change?

NO.

How was this patch tested?

UT and cluster.

codecov · 2023-07-04T14:38:16Z

Codecov Report

Merging #1678 (063bc9c) into main (de0fd8c) will increase coverage by 0.05%.
The diff coverage is 29.73%.

❗ Current head 063bc9c differs from pull request most recent head e6ff705. Consider uploading reports for the commit e6ff705 to get more accurate results

@@            Coverage Diff             @@
##             main    #1678      +/-   ##
==========================================
+ Coverage   46.22%   46.27%   +0.05%     
==========================================
  Files         161      161              
  Lines        9957     9990      +33     
  Branches      920      924       +4     
==========================================
+ Hits         4602     4622      +20     
- Misses       5051     5061      +10     
- Partials      304      307       +3

Impacted Files	Coverage Δ
...java/org/apache/celeborn/common/meta/FileInfo.java	`50.00% <ø> (ø)`
...born/common/protocol/message/ControlMessages.scala	`1.53% <0.00%> (-<0.01%)`	⬇️
.../scala/org/apache/celeborn/common/util/Utils.scala	`17.66% <ø> (ø)`
...che/celeborn/common/util/CelebornHadoopUtils.scala	`19.24% <4.77%> (-66.48%)`	⬇️
...cala/org/apache/celeborn/common/CelebornConf.scala	`87.15% <83.34%> (+0.02%)`	⬆️

... and 4 files with indirect coverage changes

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

waitinfuture · 2023-07-05T02:15:02Z

worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StorageManager.scala

@@ -534,7 +534,8 @@ final private[worker] class StorageManager(conf: CelebornConf, workerSource: Abs
        val iter = hadoopFs.listStatusIterator(hdfsWorkPath)
        while (iter.hasNext) {
          val fileStatus = iter.next()
-          if (!appIds.contains(fileStatus.getPath.getName)) {
+          if (!appIds.contains(fileStatus.getPath.getName)


getModificationTime will not reflect the change in nested directory. For example I have path /tmp/test/, then I upload a new file into /tmp/test, the modified time of /tmp will not change.
IMO, HDFS directory does not belong to worker, maybe we should let Master to clean hdfs. cc @pan3793 @RexXiong @AngersZhuuuu

AngersZhuuuu · 2023-07-05T03:52:44Z

We'd better change the doc of hdfs path configuration to mention this change and let user make sure the configuration is same in master and worker side

FMX · 2023-07-05T04:27:07Z

Updated.

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

pan3793 · 2023-07-05T05:46:46Z

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

@@ -1072,4 +1074,20 @@ object Utils extends Logging {
    }
    labelPart(0).trim -> labelPart(1).trim
  }
+
+  def getHadoopFS(conf: CelebornConf): FileSystem = {


How about moving it to CelebornHadoopUtils? and we should use CelebornHadoopUtils#newConfiguration instead of new Configuration()

Here needs an empty configuration so that celeborn conf can override pre-defined settings. So It can not be moved to CelebornHadoopUtils.

I see, the configuration priority here is

CelebornConf

hardcoded

core-site.xml, hdfs-site.xml

It changes the current behavior but looks reasonable. I'm wondering if we can change the CelebornHadoopUtils#newConfiguration and pass a Map to achieve this ability.

And, we'd better to document this behavior in some place

Comments addressed.

pan3793 · 2023-07-05T05:54:02Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

-      .withAlternative("celeborn.storage.hdfs.dir")
-      .categories("worker")
+    buildConf("celeborn.storage.hdfs.dir")
+      .withAlternative("celeborn.worker.storage.hdfs.dir")


seems we don't need this alternative, it was called celeborn.storage.hdfs.dir in 0.2

…scala Update common/src/main/scala/org/apache/celeborn/common/util/Utils.scala Co-authored-by: Cheng Pan <[email protected]>

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

RexXiong · 2023-07-05T06:43:05Z

...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java

@@ -138,6 +138,12 @@ public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, l
    partitionTotalFileCount.add(fileCount);
  }

+  public Set<String> getActiveAppIds() {


Better to use appHeartbeatTime map to get active appIds. Master may encounter corner case.

IMO, We can directly use appHeartbeatTime keySet.

IMO, We can directly use appHeartbeatTime keySet.

+1, one application may have many shuffleKeys

RexXiong · 2023-07-05T06:49:19Z

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

@@ -653,11 +656,33 @@ private[celeborn] class Master(
      override def run(): Unit = {
        statusSystem.handleAppLost(appId, requestId)
        logInfo(s"Removed application $appId")
+        // only leader can clean hdfs dirs
+        if (conf.hasHDFSStorage && !conf.hdfsDir.isEmpty) {
+          cleanExpiredAppDirsOnHDFS()


This may cost lot as applications may lost frequently in a big cluster, we would better not clean expired app dirs in handleApplicationLost, instead we need do this in timeoutDeadApplications after we handleApplicationLost. And if this cost lot, I suggest we can cache this list and refresh every x(3) min turn?

Sounds reasonable. I'll move the clean logic to timeoutDeadApplications. I think listing directories won't cost a lot.

This may cost lot as applications may lost frequently in a big cluster, we would better not clean expired app dirs in handleApplicationLost, instead we need do this in timeoutDeadApplications after we handleApplicationLost. And if this cost lot, I suggest we can cache this list and refresh every x(3) min turn?

I also think it's not a good idea to call cleanExpiredAppDirsOnHDFS when app lost. IMO we can just use forwardMessageThread.scheduleAtFixedRate to check like checkForWorkerTimeOutTask and checkForApplicationTimeOutTask. Also we need to change forwardMessageThread from single thread to multiple threads.

Also, we need to ensure that the leader has replayed all raft logs before cleanup.

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala

…HadoopUtils.scala

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala

…HadoopUtils.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

pan3793

LGTM, except to log on deleting

pan3793 · 2023-07-05T11:43:00Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

@@ -1509,6 +1510,14 @@ object CelebornConf extends Logging {
      .timeConf(TimeUnit.MILLISECONDS)
      .createWithDefaultString("300s")

+  val HDFS_REMNANTDIRS_TIMEOUT: ConfigEntry[Long] =
+    buildConf("celeborn.master.hdfs.remnantDirs.timeout")


The storage namespace should be used, and we'd better emphasize it applies to app level in the name

Suggested change

buildConf("celeborn.master.hdfs.remnantDirs.timeout")

buildConf("celeborn.master.storage.hdfs.appDirs.expiredDuration")

Or celeborn.master.storage.hdfs.appRemnantDirs.expiredDuration

Check the ding-ding group, I explained the reason why I choose this name.

@AngersZhuuuu do you have suggestion for its name?

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

…r/Master.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

…r/Master.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

…r/Master.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

…r/Master.scala

pan3793 · 2023-07-05T11:58:26Z

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

+          val startTime = System.currentTimeMillis()
+          val fileStatus = iter.next()
+          if (!statusSystem.appHeartbeatTime.containsKey(fileStatus.getPath.getName)) {
+            hadoopFs.delete(fileStatus.getPath, true)


we should try catch the IOException and print the failed to delete dir in case HDFS is abnormal or some permission deny issues

# Conflicts: # master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

client/src/main/java/org/apache/celeborn/client/ShuffleClient.java

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

### What changes were proposed in this pull request? Make Celeborn leader clean expired app dirs on HDFS when an application is Lost. ### Why are the changes needed? If Celeborn is working on HDFS, the storage manager starts and cleans expired app directories, and the newly created worker will want to delete any unknown app directories. This will cause using app directories to be deleted unexpectedly. ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? UT and cluster. Closes #1678 from FMX/CELEBORN-764. Lead-authored-by: mingji <[email protected]> Co-authored-by: Cheng Pan <[email protected]> Co-authored-by: Ethan Feng <[email protected]> Signed-off-by: zky.zhoukeyong <[email protected]> (cherry picked from commit d0ecf83) Signed-off-by: zky.zhoukeyong <[email protected]>

waitinfuture reviewed Jul 5, 2023

View reviewed changes

FMX force-pushed the CELEBORN-764 branch 2 times, most recently from dec6671 to 998c20d Compare July 5, 2023 03:42

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

FMX and others added 3 commits July 5, 2023 14:39

[CELEBORN-764] Fix celeborn on HDFS might clean using app directories.

c6b50e3

Update common/src/main/scala/org/apache/celeborn/common/CelebornConf.…

c407201

…scala Update common/src/main/scala/org/apache/celeborn/common/util/Utils.scala Co-authored-by: Cheng Pan <[email protected]>

update

d4e3595

FMX force-pushed the CELEBORN-764 branch from 93cfd94 to d4e3595 Compare July 5, 2023 06:39

update.

113f7df

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala Outdated Show resolved Hide resolved

pan3793 and others added 4 commits July 5, 2023 14:43

Update common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

cd37620

update.

b21b056

Merge remote-tracking branch 'origin/CELEBORN-764' into CELEBORN-764

344787e

update.

dd6f55d

RexXiong reviewed Jul 5, 2023

View reviewed changes

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

address comments.

84ceda8

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

Update common/src/main/scala/org/apache/celeborn/common/util/Celeborn…

acbcdc8

…HadoopUtils.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

Update common/src/main/scala/org/apache/celeborn/common/util/Celeborn…

59b99c3

…HadoopUtils.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

Add new configs to avoid checking frequently.

393bb53

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

pan3793 approved these changes Jul 5, 2023

View reviewed changes

pan3793 reviewed Jul 5, 2023

View reviewed changes

refine.

22b6989

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

Update master/src/main/scala/org/apache/celeborn/service/deploy/maste…

9e47b1a

…r/Master.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

Update master/src/main/scala/org/apache/celeborn/service/deploy/maste…

ff715b8

…r/Master.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

Update master/src/main/scala/org/apache/celeborn/service/deploy/maste…

59d0869

…r/Master.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

Update master/src/main/scala/org/apache/celeborn/service/deploy/maste…

2999a49

…r/Master.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

FMX added 2 commits July 5, 2023 19:59

refine.

1d793b7

Merge remote-tracking branch 'origin/CELEBORN-764' into CELEBORN-764

d18430d

# Conflicts: # master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

refine.

2fbcc57

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

client/src/main/java/org/apache/celeborn/client/ShuffleClient.java Outdated Show resolved Hide resolved

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/CelebornHadoopUtils.scala Outdated Show resolved Hide resolved

FMX added 2 commits July 5, 2023 20:42

refine.

9499121

refine.

1643cb6

pan3793 reviewed Jul 5, 2023

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala Outdated Show resolved Hide resolved

FMX added 3 commits July 5, 2023 20:59

refine.

93689d9

refine.

063bc9c

refine.

e6ff705

waitinfuture closed this in d0ecf83 Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-764] Fix celeborn on HDFS might clean using app directories. #1678

[CELEBORN-764] Fix celeborn on HDFS might clean using app directories. #1678

FMX commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

waitinfuture Jul 5, 2023

AngersZhuuuu commented Jul 5, 2023

FMX commented Jul 5, 2023

pan3793 Jul 5, 2023

FMX Jul 5, 2023

pan3793 Jul 5, 2023 •

edited

Loading

pan3793 Jul 5, 2023

FMX Jul 5, 2023

pan3793 Jul 5, 2023

RexXiong Jul 5, 2023

RexXiong Jul 5, 2023

waitinfuture Jul 5, 2023

RexXiong Jul 5, 2023

FMX Jul 5, 2023

waitinfuture Jul 5, 2023

waitinfuture Jul 5, 2023

pan3793 left a comment

pan3793 Jul 5, 2023 •

edited

Loading

FMX Jul 5, 2023

pan3793 Jul 5, 2023

pan3793 Jul 5, 2023

FMX Jul 5, 2023

	buildConf("celeborn.master.hdfs.remnantDirs.timeout")
	buildConf("celeborn.master.storage.hdfs.appDirs.expiredDuration")

[CELEBORN-764] Fix celeborn on HDFS might clean using app directories. #1678

[CELEBORN-764] Fix celeborn on HDFS might clean using app directories. #1678

Conversation

FMX commented Jul 4, 2023 • edited Loading

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 bot commented Jul 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AngersZhuuuu commented Jul 5, 2023

FMX commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Jul 5, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 left a comment

Choose a reason for hiding this comment

pan3793 Jul 5, 2023 • 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

FMX commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

pan3793 Jul 5, 2023 •

edited

Loading

pan3793 Jul 5, 2023 •

edited

Loading