[CELEBORN-914] Support memory file storage by FMX · Pull Request #2300 · apache/celeborn

FMX · 2024-02-19T01:20:36Z

What changes were proposed in this pull request?

To support memory file storage.

Why are the changes needed?

To improve shuffle performance for small shuffle files.

Design doc: https://docs.google.com/document/d/1SM-oOM0JHEIoRHTYhE9PYH60_1D3NMxDR50LZIM7uW0/edit?usp=sharing

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA and manually test on a cluster.

codecov · 2024-02-19T10:57:16Z

Codecov Report

Attention: Patch coverage is 73.15436% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 49.33%. Comparing base (21d5698) to head (52cd7d8).
Report is 8 commits behind head on main.

Files	Patch %	Lines
...java/org/apache/celeborn/common/meta/FileInfo.java	62.86%	10 Missing and 3 partials ⚠️
...rg/apache/celeborn/common/meta/MemoryFileInfo.java	52.95%	8 Missing ⚠️
...rg/apache/celeborn/common/meta/ReduceFileMeta.java	70.00%	6 Missing ⚠️
...he/celeborn/common/util/ShuffleBlockInfoUtils.java	83.34%	0 Missing and 6 partials ⚠️
...g/apache/celeborn/common/protocol/StorageInfo.java	50.00%	2 Missing ⚠️
...cala/org/apache/celeborn/common/CelebornConf.scala	88.24%	1 Missing and 1 partial ⚠️
...org/apache/celeborn/common/util/PbSerDeUtils.scala	0.00%	2 Missing ⚠️
...e/celeborn/common/network/buffer/ChunkBuffers.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2300      +/-   ##
==========================================
+ Coverage   48.96%   49.33%   +0.38%     
==========================================
  Files         209      211       +2     
  Lines       13102    13238     +136     
  Branches     1134     1149      +15     
==========================================
+ Hits         6414     6530     +116     
- Misses       6270     6276       +6     
- Partials      418      432      +14

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

client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java

RexXiong · 2024-03-11T02:25:57Z

common/src/main/java/org/apache/celeborn/common/meta/FileInfo.java

  }
+
+  public boolean addStream(long streamId) {
+    ReduceFileMeta reduceFileMeta = (ReduceFileMeta) fileMeta;


should not use actual type ReduceFileMeta directly, Can we delegate stream operation to FileMeta or use instanceof?

RexXiong · 2024-03-11T09:08:58Z

common/src/main/java/org/apache/celeborn/common/meta/MemoryFileInfo.java

    return length;
  }
+
+  public void addLength(int length) {


MemoryInfo.addLength seems the same like DiskFileInfo.updateBytesFlushed

RexXiong · 2024-03-11T09:26:04Z

common/src/main/java/org/apache/celeborn/common/meta/ManagedBuffers.java

+import org.apache.celeborn.common.network.buffer.ManagedBuffer;
+
+public interface ManagedBuffers {
+  int numChunks();


add @OverRide to all subclasses

RexXiong · 2024-03-11T09:27:23Z

common/src/main/java/org/apache/celeborn/common/network/buffer/MemoryFileManagedBuffers.java

+  private final int numChunks;
+  private final CompositeByteBuf buffer;
+
+  public MemoryFileManagedBuffers(MemoryFileInfo memoryFileInfo) {


We can create BaseChunk/ReduceManagedBuffers to eliminate duplicate codes

RexXiong · 2024-03-11T09:38:32Z

common/src/main/java/org/apache/celeborn/common/util/ShuffleBlockInfoUtils.java

    return indexMap;
  }
+
+  public static void reorganizeBuffer(


reorganizeBuffer -> sortBufferByMapRange?

RexXiong · 2024-03-11T09:43:08Z

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

      .categories("worker")
-      .doc("Max ratio of direct memory to store shuffle data")
-      .version("0.2.0")
+      .doc("Max ratio of direct memory to store shuffle data.")


0.4.1->0.5, what's the meaning about the default value

RexXiong · 2024-03-11T10:07:38Z

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

+        null)
+    }
+
+    if (location.getStorageInfo.localDiskAvailable() || location.getStorageInfo.HDFSAvailable()) {


RexXiong · 2024-03-11T14:56:26Z

pom.xml

              <include>**/*Test*.*</include>
            </includes>
            <reportsDirectory>${project.build.directory}/surefire-reports</reportsDirectory>
-            <argLine>${argLine} -ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=128m ${extraJavaTestArgs} -Dio.netty.tryReflectionSetAccessible=true</argLine>


Don't change that in this pr

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

RexXiong · 2024-03-12T02:21:55Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

+        createFile(writerContext);
+
+    // Reduce partition data writers support memory storage now
+    if (supportInMemory && createFileResult._1() != null) {


IMO better to delegate the actual writing and eviction to something like a TieredFileStorage. This would make the PartitionDataWriter more clear.

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

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

client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

...c/main/java/org/apache/celeborn/service/deploy/worker/storage/ReducePartitionDataWriter.java

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

worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala

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

# Conflicts: # client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java # worker/src/main/scala/org/apache/celeborn/service/deploy/worker/FetchHandler.scala

codecov-commenter · 2024-04-07T01:49:31Z

Codecov Report

Attention: Patch coverage is 9.49367% with 143 lines in your changes are missing coverage. Please review.

Project coverage is 38.98%. Comparing base (121395f) to head (53555a1).
Report is 51 commits behind head on main.

❗ Current head 53555a1 differs from pull request most recent head 137ab52

Please upload reports for the commit 137ab52 to get more accurate results.

Files	Patch %	Lines
...he/celeborn/common/util/ShuffleBlockInfoUtils.java	0.00%	36 Missing ⚠️
...java/org/apache/celeborn/common/meta/FileInfo.java	0.00%	35 Missing ⚠️
...rg/apache/celeborn/common/meta/MemoryFileInfo.java	0.00%	24 Missing ⚠️
...rg/apache/celeborn/common/meta/ReduceFileMeta.java	0.00%	20 Missing ⚠️
...leborn/common/network/buffer/FileChunkBuffers.java	0.00%	6 Missing ⚠️
...born/common/network/buffer/MemoryChunkBuffers.java	0.00%	6 Missing ⚠️
...cala/org/apache/celeborn/common/CelebornConf.scala	68.43%	6 Missing ⚠️
...e/celeborn/common/network/buffer/ChunkBuffers.java	0.00%	4 Missing ⚠️
.../org/apache/celeborn/common/meta/DiskFileInfo.java	0.00%	2 Missing ⚠️
...g/apache/celeborn/common/protocol/StorageInfo.java	50.00%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2300      +/-   ##
==========================================
- Coverage   40.17%   38.98%   -1.19%     
==========================================
  Files         218      219       +1     
  Lines       13742    13547     -195     
  Branches     1214     1191      -23     
==========================================
- Hits         5520     5280     -240     
- Misses       7905     7966      +61     
+ Partials      317      301      -16

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

# Conflicts: # worker/src/test/java/org/apache/celeborn/service/deploy/worker/storage/local/DiskReducePartitionDataWriterSuiteJ.java

common/src/main/java/org/apache/celeborn/common/meta/ReduceFileMeta.java

common/src/main/java/org/apache/celeborn/common/network/buffer/FileChunkBuffers.java

common/src/main/java/org/apache/celeborn/common/network/buffer/ChunkBuffers.java

common/src/main/java/org/apache/celeborn/common/protocol/StorageInfo.java

waitinfuture · 2024-05-21T03:52:07Z

common/src/main/java/org/apache/celeborn/common/util/ShuffleBlockInfoUtils.java

+    for (int i = startMapIndex; i < endMapIndex; i++) {
+      List<ShuffleBlockInfo> blockInfos = indexMap.get(i);
+      if (blockInfos != null) {
+        for (ShuffleBlockInfo blockInfo : blockInfos) {


Just calculate start and end, then slice once from sortedByteBuf. Check contiguous during calculation.

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

waitinfuture · 2024-05-21T06:04:55Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

    }
    // trigger resume
-    if (memoryUsage < resumeThreshold) {
+    if (workerMemoryUsageRatio() < resumeRatio) {


Just use memoryUsage / (double) (maxDirectMemory) here to avoid duplicate calculation

There is a gauge to export this usage ratio to Grafana.

worker/src/main/java/org/apache/celeborn/service/deploy/worker/memory/MemoryManager.java

waitinfuture · 2024-05-21T08:02:02Z

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

+          MemoryManager.instance().incrementDiskBuffer(numBytes);
+          // read flush buffer to generate correct chunk offsets
+          // data header layout (mapId, attemptId, nextBatchId, length)
+          ByteBuffer headerBuf = ByteBuffer.allocate(16);


Better to check if flushBuffer exceeds chunkSize, if yes, do the following logic, else just use numBytes.

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

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

worker/src/main/scala/org/apache/celeborn/service/deploy/worker/FetchHandler.scala

client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java

common/src/main/java/org/apache/celeborn/common/meta/FileInfo.java

common/src/main/java/org/apache/celeborn/common/meta/MemoryFileInfo.java

common/src/main/java/org/apache/celeborn/common/meta/ReduceFileMeta.java

common/src/main/java/org/apache/celeborn/common/util/ShuffleBlockInfoUtils.java

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/ChunkStreamManager.java

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

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

# Conflicts: # assets/grafana/celeborn-dashboard.json

waitinfuture

LGTM, thanks for the effort! Merging to main(v0.5.0). Please add more UTs for this feature in followup PRs.

s0nskar · 2024-08-14T14:10:47Z

@FMX metrics added by this PR is not added to the Celeborn website Monitoring page. Also, should we start adding such changes in the release notes as well. WDYT?

…nitoring.md ### What changes were proposed in this pull request? Adding documentation for missing memory file storage metrics. ### Why are the changes needed? Few new metrics were added in #2300 but they were missing their documentation in monitoring.md ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? NA Closes #2705 from s0nskar/memory_metrics. Authored-by: Sanskar Modi <sanskarmodi97@gmail.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…nitoring.md Adding documentation for missing memory file storage metrics. Few new metrics were added in #2300 but they were missing their documentation in monitoring.md NO NA Closes #2705 from s0nskar/memory_metrics. Authored-by: Sanskar Modi <sanskarmodi97@gmail.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com> (cherry picked from commit b7027b6) Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…nitoring.md ### What changes were proposed in this pull request? Adding documentation for missing memory file storage metrics. ### Why are the changes needed? Few new metrics were added in apache#2300 but they were missing their documentation in monitoring.md ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? NA Closes apache#2705 from s0nskar/memory_metrics. Authored-by: Sanskar Modi <sanskarmodi97@gmail.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…Ids of worker service log in startup document ### What changes were proposed in this pull request? Add `emptyFilePrimaryIds` and `emptyFileReplicaIds` of worker service log in startup document. ### Why are the changes needed? #2300 has added `emptyFilePrimaryIds` and `emptyFileReplicaIds` of startup log of for worker service in `Controller`, which should also add into the log of startup document. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes #2774 from SteNicholas/CELEBORN-914. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…Ids of worker service log in startup document ### What changes were proposed in this pull request? Add `emptyFilePrimaryIds` and `emptyFileReplicaIds` of worker service log in startup document. ### Why are the changes needed? #2300 has added `emptyFilePrimaryIds` and `emptyFileReplicaIds` of startup log of for worker service in `Controller`, which should also add into the log of startup document. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes #2774 from SteNicholas/CELEBORN-914. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com> (cherry picked from commit c5ff12b) Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…Ids of worker service log in startup document ### What changes were proposed in this pull request? Add `emptyFilePrimaryIds` and `emptyFileReplicaIds` of worker service log in startup document. ### Why are the changes needed? apache#2300 has added `emptyFilePrimaryIds` and `emptyFileReplicaIds` of startup log of for worker service in `Controller`, which should also add into the log of startup document. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes apache#2774 from SteNicholas/CELEBORN-914. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…nitoring.md ### What changes were proposed in this pull request? Adding documentation for missing memory file storage metrics. ### Why are the changes needed? Few new metrics were added in apache#2300 but they were missing their documentation in monitoring.md ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? NA Closes apache#2705 from s0nskar/memory_metrics. Authored-by: Sanskar Modi <sanskarmodi97@gmail.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

…Ids of worker service log in startup document ### What changes were proposed in this pull request? Add `emptyFilePrimaryIds` and `emptyFileReplicaIds` of worker service log in startup document. ### Why are the changes needed? apache#2300 has added `emptyFilePrimaryIds` and `emptyFileReplicaIds` of startup log of for worker service in `Controller`, which should also add into the log of startup document. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes apache#2774 from SteNicholas/CELEBORN-914. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>

FMX marked this pull request as draft February 19, 2024 01:20

FMX force-pushed the B914 branch 2 times, most recently from fdfff8d to a0e672f Compare February 19, 2024 08:18

FMX force-pushed the B914 branch 5 times, most recently from efdf6e3 to 6cc81ac Compare February 22, 2024 02:25

FMX marked this pull request as ready for review February 26, 2024 06:42

FMX force-pushed the B914 branch 2 times, most recently from a29e592 to 462f5c4 Compare February 29, 2024 08:58

RexXiong reviewed Mar 11, 2024

View reviewed changes

RexXiong reviewed Mar 12, 2024

View reviewed changes

FMX added 2 commits March 12, 2024 16:20

[CELEBORN-914] Support memory file storage

608507c

Address comments and refine.

d311a84

FMX force-pushed the B914 branch from b77e796 to d311a84 Compare March 12, 2024 09:01

RexXiong reviewed Mar 14, 2024

View reviewed changes

FMX added 8 commits March 20, 2024 15:26

Merge branch 'main' into B914

01c0eac

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

address comments

660ce49

fix ut

2bea86b

update

52cd7d8

Merge branch 'refs/heads/main' into B914

edf3a0d

# Conflicts: # client/src/main/java/org/apache/celeborn/client/read/CelebornInputStream.java # worker/src/main/scala/org/apache/celeborn/service/deploy/worker/FetchHandler.scala

format

d971122

address comments

7513106

update

bc4fce8

FMX added 3 commits April 7, 2024 17:45

Merge branch 'refs/heads/main' into B914

53555a1

fix ut

b7a6149

Merge branch 'refs/heads/main' into B914

b93b652

# Conflicts: # worker/src/test/java/org/apache/celeborn/service/deploy/worker/storage/local/DiskReducePartitionDataWriterSuiteJ.java

waitinfuture reviewed May 21, 2024

View reviewed changes

address comments

39096e8

FMX force-pushed the B914 branch from 2a920a9 to 39096e8 Compare May 21, 2024 08:34

waitinfuture reviewed May 21, 2024

View reviewed changes

FMX added 7 commits May 21, 2024 21:02

address comments

ef0a126

fix ut

8c25a4b

remove unused field

8d19282

refine codes

0002292

compile

224ee90

fix ut

6f2f27e

address comments

e5a2d55

waitinfuture reviewed May 22, 2024

View reviewed changes

FMX added 4 commits May 22, 2024 20:56

address comments

2459731

fix ut

68add31

Merge branch 'main' into B914

f0dc7ce

# Conflicts: # assets/grafana/celeborn-dashboard.json

fix

137ab52

waitinfuture approved these changes May 23, 2024

View reviewed changes

FMX closed this in 89d56c9 May 23, 2024

s0nskar mentioned this pull request Aug 26, 2024

[CELEBORN-914][FOLLOWUP] Adding metrics for memory file storage in monitoring.md #2705

Closed

SteNicholas mentioned this pull request Sep 30, 2024

[CELEBORN-914][FOLLOWUP] Add emptyFilePrimaryIds and emptyFileReplicaIds of worker service log in startup document #2774

Closed

Conversation

FMX commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

codecov bot commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Apr 7, 2024 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waitinfuture left a comment

Choose a reason for hiding this comment

Uh oh!

s0nskar commented Aug 14, 2024

FMX commented Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 19, 2024 •

edited

Loading

codecov-commenter commented Apr 7, 2024 •

edited by codecov bot

Loading