[#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable #1659

xumanbu · 2024-04-18T07:48:27Z

What changes were proposed in this pull request?

FileSegmentManagedBuffer#nioByteBuffer add a buffer to hold read file result to avoid multiple read file

Why are the changes needed?

Fix: #1658

Does this PR introduce any user-facing change?

No.

How was this patch tested?

existed UT

xumanbu · 2024-04-18T08:26:50Z

PTAL. @dingshun3016 @zuston @jerqi

github-actions · 2024-04-18T08:32:03Z

Test Results

2 398 files +14 2 398 suites +14 4h 38m 33s ⏱️ + 6m 5s
921 tests + 2 920 ✅ + 2 1 💤 ±0 0 ❌ ±0
10 689 runs +23 10 675 ✅ +23 14 💤 ±0 0 ❌ ±0

Results for commit 8740feb. ± Comparison against base commit 5ab625b.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.

org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType

org.apache.uniffle.common.netty.buffer.FileSegmentManagedBufferTest ‑ testNioByteBuffer{File}
org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType{ServerType}[1]
org.apache.uniffle.shuffle.manager.ShuffleManagerServerFactoryTest ‑ testShuffleManagerServerType{ServerType}[2]

♻️ This comment has been updated with latest results.

zuston · 2024-04-18T08:43:28Z

Could you help review this? @rickyma

rickyma

Left some comments. I'm OK with this patch. But you need to test it more carefully. Because this is used in gRPC mode. It won't be invoked in Netty mode. So I don't know how it actually performs in a production environment. Because I currently use Netty in my environment.

rickyma · 2024-04-18T13:03:21Z

common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java

@@ -39,6 +39,8 @@ public class FileSegmentManagedBuffer extends ManagedBuffer {
  private final File file;
  private final long offset;
  private final int length;
+  private boolean isFilled;


Maybe we don't need isFilled at all. I think it can be replaced by readByteBuffer != null.

The readByteBuffer may be allocated but not filled with bytes, indicating a failure to read the file. Therefore, I believe adding another state for reliability is necessary.

rickyma · 2024-04-18T13:06:28Z

common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java

    } catch (IOException e) {
-      String errorMessage = "Error in reading " + this;
+      String errorMessage = "Error in reading file " + file + " " + this;


It's better if we use String.format.

rickyma · 2024-04-18T13:06:49Z

common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java

      try {
        if (channel != null) {
          long size = channel.size();
-          errorMessage = "Error in reading " + this + " (actual file length " + size + ")";
+          errorMessage =


zuston · 2024-04-18T13:50:47Z

Left some comments. I'm OK with this patch. But you need to test it more carefully. Because this is used in gRPC mode. It won't be invoked in Netty mode. So I don't know how it actually performs in a production environment. Because I currently use Netty in my environment.

Oh I thought this is netty. I will take another look tomorrow.

jerqi · 2024-04-19T12:22:18Z

Could you add a ut if this is a fix?

xumanbu · 2024-04-29T06:15:45Z

Could you add a ut if this is a fix?

ok. added.

zuston

Sorry for the late reply.

Nice improvement, this is useful for AQE skew tasks. Left minor comments. @xumanbu

zuston · 2024-04-29T06:18:23Z

common/src/main/java/org/apache/uniffle/common/netty/buffer/FileSegmentManagedBuffer.java

-      return buf;
+      readByteBuffer.flip();
+      isFilled = true;
+      return readByteBuffer;


Can we make this as readOnlyBuffer? return buf.asReadOnlyBuffer();

I have tested it, but it doesn't work. ReadOnlyBuffer will throw a ReadOnlyBufferException when calling buffer.array().

Pity. I want to ensure the readBuffer will not be changed after cached.

jerqi · 2025-01-02T07:17:43Z

@xumanbu @zuston Will you continue this pull request?

jerqi · 2025-01-20T06:10:59Z

@zuston @xumanbu Do we need to continue this pull request?

xumanbu · 2025-01-20T10:26:12Z

@zuston @xumanbu Do we need to continue this pull request?

I think this is necessary to continue, I'll pick it up.

…d result cacheable

xumanbu · 2025-01-23T08:58:30Z

@jerqi mistakenly discarded changes before committing. I've recommitted them. Could you help me reopen this PR?

xumanbu · 2025-01-23T10:44:51Z

@zuston @rickyma Please help me see has any other issues

zuston

+1 for this. Thanks for your contribution. If no one reject this, I will merge this in the next 2 days.

jerqi · 2025-01-26T04:50:25Z

@rickyma Do u have another suggestion?

rickyma · 2025-01-26T18:31:10Z

@rickyma Do u have another suggestion?

Nope. LGTM. But I did not test this.

zuston · 2025-01-27T01:24:16Z

Ok. Let's merge this.

zuston changed the title ~~[#1658] fix(server) FileSegmentManagedBuffer#nioByteBuffer read result cacheable~~ [#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable Apr 18, 2024

rickyma requested changes Apr 18, 2024

View reviewed changes

zuston reviewed Apr 29, 2024

View reviewed changes

xumanbu closed this Jan 23, 2025

xumanbu force-pushed the 1658_FileSegmentManagedBuffer branch from 8740feb to 08e7397 Compare January 23, 2025 08:22

[apache#1658] improvement: FileSegmentManagedBuffer#nioByteBuffer rea…

4271781

…d result cacheable

jerqi reopened this Jan 23, 2025

xumanbu requested a review from rickyma January 23, 2025 10:42

zuston approved these changes Jan 24, 2025

View reviewed changes

rickyma approved these changes Jan 26, 2025

View reviewed changes

zuston merged commit 3590940 into apache:master Jan 27, 2025
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable #1659

[#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable #1659

xumanbu commented Apr 18, 2024 •

edited

Loading

xumanbu commented Apr 18, 2024

github-actions bot commented Apr 18, 2024 •

edited

Loading

zuston commented Apr 18, 2024

rickyma left a comment •

edited

Loading

rickyma Apr 18, 2024

xumanbu Apr 29, 2024

rickyma Apr 18, 2024

xumanbu Apr 29, 2024

rickyma Apr 18, 2024

xumanbu Apr 29, 2024

zuston commented Apr 18, 2024

jerqi commented Apr 19, 2024

xumanbu commented Apr 29, 2024

zuston left a comment •

edited

Loading

zuston Apr 29, 2024

xumanbu Apr 29, 2024

zuston Apr 29, 2024

jerqi commented Jan 2, 2025

jerqi commented Jan 20, 2025

xumanbu commented Jan 20, 2025

xumanbu commented Jan 23, 2025

xumanbu commented Jan 23, 2025

zuston left a comment

jerqi commented Jan 26, 2025

rickyma commented Jan 26, 2025

zuston commented Jan 27, 2025

[#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable #1659

[#1658] fix(server): FileSegmentManagedBuffer#nioByteBuffer read result cacheable #1659

Conversation

xumanbu commented Apr 18, 2024 • 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?

xumanbu commented Apr 18, 2024

github-actions bot commented Apr 18, 2024 • edited Loading

Test Results

zuston commented Apr 18, 2024

rickyma left a comment • 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

zuston commented Apr 18, 2024

jerqi commented Apr 19, 2024

xumanbu commented Apr 29, 2024

zuston left a comment • 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

jerqi commented Jan 2, 2025

jerqi commented Jan 20, 2025

xumanbu commented Jan 20, 2025

xumanbu commented Jan 23, 2025

xumanbu commented Jan 23, 2025

zuston left a comment

Choose a reason for hiding this comment

jerqi commented Jan 26, 2025

rickyma commented Jan 26, 2025

zuston commented Jan 27, 2025

xumanbu commented Apr 18, 2024 •

edited

Loading

github-actions bot commented Apr 18, 2024 •

edited

Loading

rickyma left a comment •

edited

Loading

zuston left a comment •

edited

Loading