You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "otterc (via GitHub)" <gi...@apache.org> on 2023/11/02 18:41:34 UTC
[PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
otterc opened a new pull request, #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065
### What changes were proposed in this pull request?
This change makes the maximum default number of Netty threads configurable. Previously, this value was hardcoded to 64, which could be small for certain environments. While it's possible to configure the number of Netty server and client threads individually for each module, providing an option to increase the default value offers greater convenience.
### Why are the changes needed?
The change offers convenience.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added a UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#issuecomment-1791785747
Hi @otterc , thanks for the PR! Please run `UPDATE=1 build/mvn clean test -pl common -am -Pspark-3.3 -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite` to automatically update the md files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#discussion_r1381171496
##########
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala:
##########
@@ -476,14 +476,14 @@ object Utils extends Logging {
hostPortParseResults.get(hostPort)
}
- private val MAX_DEFAULT_NETTY_THREADS = 64
+ private var maxDefaultNettyThreads = 64
Review Comment:
Do we want to simply eliminate this variable ?
Pass `conf` to `defaultNumThreads` and fetch from there ? Or move the method into `fromCelebornConf` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#issuecomment-1791842619
@otterc, you could refer to contributing guide: [CONTRIBUTING.md#code-style](https://github.com/apache/incubator-celeborn/blob/main/CONTRIBUTING.md#code-style).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#issuecomment-1791841786
@otterc, you could run `UPDATE=1 build/mvn clean test -pl common -am -Dtest=none -DwildcardSuites=org.apache.celeborn.ConfigurationSuite` command to generate configuration document of introduced config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#issuecomment-1791882299
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2065](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e188051) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/7b185a2562857b35ae4fd6bed7026ebd616eeda9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7b185a2) will **decrease** coverage by `0.08%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head e188051 differs from pull request most recent head cb4b26b. Consider uploading reports for the commit cb4b26b to get more accurate results
```diff
@@ Coverage Diff @@
## main #2065 +/- ##
==========================================
- Coverage 46.91% 46.82% -0.08%
==========================================
Files 165 165
Lines 10586 10594 +8
Branches 963 963
==========================================
- Hits 4965 4960 -5
- Misses 5299 5310 +11
- Partials 322 324 +2
```
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | `87.73% <100.00%> (+0.04%)` | :arrow_up: |
| [.../scala/org/apache/celeborn/common/util/Utils.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3V0aWwvVXRpbHMuc2NhbGE=) | `16.90% <100.00%> (+0.17%)` | :arrow_up: |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2065/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2065:
URL: https://github.com/apache/incubator-celeborn/pull/2065#discussion_r1381171496
##########
common/src/main/scala/org/apache/celeborn/common/util/Utils.scala:
##########
@@ -476,14 +476,14 @@ object Utils extends Logging {
hostPortParseResults.get(hostPort)
}
- private val MAX_DEFAULT_NETTY_THREADS = 64
+ private var maxDefaultNettyThreads = 64
Review Comment:
Do we want to simply eliminate this variable ?
Pass `conf` to `defaultNumThreads` and fetch from there ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1107] Make the max default number of netty threads configurable [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2065: [CELEBORN-1107] Make the max default number of netty threads configurable
URL: https://github.com/apache/incubator-celeborn/pull/2065
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org