Skip to content

[CELEBORN-1993] CelebornConf introduces celeborn.<module>.io.threads to specify number of threads used in the client thread pool#3245

Closed
SteNicholas wants to merge 1 commit into
apache:mainfrom
SteNicholas:CELEBORN-1993
Closed

[CELEBORN-1993] CelebornConf introduces celeborn.<module>.io.threads to specify number of threads used in the client thread pool#3245
SteNicholas wants to merge 1 commit into
apache:mainfrom
SteNicholas:CELEBORN-1993

Conversation

@SteNicholas

@SteNicholas SteNicholas commented May 7, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

CelebornConf introduces celeborn.<module>.io.threads to specify number of threads used in the client thread pool.

Why are the changes needed?

ShuffleClientImpl and FlinkShuffleClientImpl use fixed configuration expression as conf.getInt("celeborn." + module + ".io.threads", 8). Therefore, CelebornConf should introduce celeborn.<module>.io.threads to specify number of threads used in the client thread pool.

Does this PR introduce any user-facing change?

CelebornConf adds celeborn.<module>.io.threads config option.

How was this patch tested?

No.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new configuration option (celeborn..io.threads) to allow users to specify the number of threads for both the server and client thread pools.

  • Updated docs/configuration/network.md with the new configuration property.
  • Modified ShuffleClientImpl and FlinkShuffleClientImpl to use conf.networkIoThreads(module) instead of a hardcoded default value.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
docs/configuration/network.md Added new configuration property for IO threads used in server and client thread pools.
client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java Replaced fixed config value with dynamic value from conf.networkIoThreads(module).
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/client/FlinkShuffleClientImpl.java Updated to use the new configuration method for retrieving IO threads.
Files not reviewed (1)
  • common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala: Language not supported
Comments suppressed due to low confidence (3)

docs/configuration/network.md:40

  • [nitpick] Consider clarifying in the description that this configuration option sets the thread count for both server and client thread pools, and explicitly mention the fallback behavior if not provided.
| celeborn.<module>.io.threads | 8 | false | Number of threads used in the server and client thread pool. If setting <module> to `data`, it works for shuffle client push and fetch data. |  |  |

client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java:225

  • [nitpick] Ensure that the method conf.networkIoThreads(module) properly validates its return value and falls back to 8 if an invalid or missing configuration is encountered.
dataTransportConf = Utils.fromCelebornConf(conf, module, conf.networkIoThreads(module));

client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/client/FlinkShuffleClientImpl.java:177

  • [nitpick] Similarly, verify that conf.networkIoThreads(module) in the FlinkShuffleClientImpl properly handles edge cases and returns a sensible default, ensuring consistency with the client implementation.
Utils.fromCelebornConf(conf, module, conf.networkIoThreads(module));

@SteNicholas

Copy link
Copy Markdown
Member Author

@FMX, @RexXiong.

Comment thread common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala
Comment thread common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala Outdated
…to specify number of threads used in the client thread pool
@SteNicholas SteNicholas changed the title [CELEBORN-1993] CelebornConf introduces celeborn.<module>.io.threads to specify number of threads used in the server and client thread pool [CELEBORN-1993] CelebornConf introduces celeborn.<module>.io.threads to specify number of threads used in the client thread pool May 11, 2025
@SteNicholas SteNicholas requested a review from RexXiong May 11, 2025 06:47
@SteNicholas

SteNicholas commented May 13, 2025

Copy link
Copy Markdown
Member Author

Ping @RexXiong, @FMX. PTAL.

Comment thread common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala
@SteNicholas SteNicholas requested a review from turboFei May 19, 2025 09:02

@turboFei turboFei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@FMX FMX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks. Merged into main(v0.6.0).

@FMX FMX closed this in fd715b4 May 20, 2025
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.

5 participants