Skip to content

Conversation

@xy2953396112
Copy link
Contributor

@xy2953396112 xy2953396112 commented Nov 22, 2025

What changes were proposed in this pull request?

Initialize the FSDataOutputStream in the DfsTierWriter, use it for data writing when the HdfsFlushTask performs flushing, and close the FSDataOutputStream when executing closeStreams.

Why are the changes needed?

Avoid allocating additional buffers When HdfsFlushTask writes data.

Does this PR resolve a correctness bug?

NO

Does this PR introduce any user-facing change?

NO

How was this patch tested?

CI

@RexXiong RexXiong changed the title [CELEBORN-2211] Avoid allocating additional buffers When HdfsFlushTas… [CELEBORN-2211] Avoid allocating additional buffers When HdfsFlushTask writes data Dec 5, 2025
@RexXiong
Copy link
Contributor

RexXiong commented Dec 5, 2025

Could you take some time to review this PR? @FMX

@FMX
Copy link
Contributor

FMX commented Dec 9, 2025

@xy2953396112 @RexXiong Just hold on a moment, I'll review this PR this Saturday.

@FMX
Copy link
Contributor

FMX commented Dec 14, 2025

@xy2953396112 I remember that your hdfs client is optimized and will not create a thread in the output stream but the oss client can not achieve that.
Maybe add a config to reuse the hdfs output stream if the user ensure that its client won't cause trouble?

Copy link
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be good choice for the oss hdfs client but I think it will be beneficial if you add a config for this.

@codecov
Copy link

codecov bot commented Dec 14, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.12%. Comparing base (ffff5bb) to head (ff9ad7f).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
...cala/org/apache/celeborn/common/CelebornConf.scala 85.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3548      +/-   ##
==========================================
+ Coverage   67.05%   67.12%   +0.08%     
==========================================
  Files         357      357              
  Lines       21779    21808      +29     
  Branches     1930     1930              
==========================================
+ Hits        14602    14637      +35     
+ Misses       6160     6156       -4     
+ Partials     1017     1015       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xy2953396112
Copy link
Contributor Author

@FMX PTAL, thanks~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants