You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "mridulm (via GitHub)" <gi...@apache.org> on 2024/04/21 07:48:15 UTC
[PR] CELEBORN-1354 Followup: split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
mridulm opened a new pull request, #2471:
URL: https://github.com/apache/celeborn/pull/2471
### What changes were proposed in this pull request?
Based on [review feedback here](https://github.com/apache/celeborn/pull/2469#discussion_r1573228448), split `rpc_app` module into two internal transport modules - `rpc_app_lifecyclemanager` and `rpc_app_client`.
These are not directly configured by users - but internally allow us to differentiate between lifecycle manager and executor
### Why are the changes needed?
auto ssl is to be configured only at lifecycle manager - doing it at executors is beneign, but not very useful (it results in creation of local files, etc).
Avoid this by handling it specifically only for lifecyclemanager.
This is based on [review feedback](https://github.com/apache/celeborn/pull/2469#discussion_r1573228448).
### Does this PR introduce _any_ user-facing change?
No, the modules are all internal and not user visible.
### How was this patch tested?
Unit tests were updated
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1574068012
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -24,11 +24,21 @@ public class TransportModuleConstants {
// RPC module used by the application components to communicate with each other
// This is used only at the application side.
+ // This is interally further split into RPC_LIFECYCLEMANAGER_MODULE and
+ // RPC_APP_CLIENT_MODULE - both of which inherit from RPC_APP_MODULE
+ // So for users, there is only RPC_APP_MODULE
public static final String RPC_APP_MODULE = "rpc_app";
// RPC module used to communicate with/between server components
// This is used both at server (master/worker) and application side.
public static final String RPC_SERVICE_MODULE = "rpc_service";
+ // See RPC_APP_MODULE for details - both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // are internal modules, and transport configs are not expected for these.
+ // For example, auto-ssl requires both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // to be in sync, though it is enabled only in RPC_LIFECYCLEMANAGER_MODULE
+ public static final String RPC_LIFECYCLEMANAGER_MODULE = "rpc_app_lifecyclemanager";
+ public static final String RPC_APP_CLIENT_MODULE = "rpc_app_client";
Review Comment:
Sounds good
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573950428
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -87,13 +87,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
this
}
+ private def warnIfInternalTransportModule(module: String, key: String): Unit = {
+ // for now, assert false to test
Review Comment:
Should remove it - I had an assertion here earlier to test/validate I was not missing anything :-)
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573952743
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1371,12 +1397,24 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
object CelebornConf extends Logging {
- val TRANSPORT_MODULE_FALLBACKS = Map(
- "rpc_service" -> "rpc",
- "rpc_app" -> "rpc",
+ val TRANSPORT_MODULE_FALLBACKS: Map[String, String] = Map(
Review Comment:
Yes, this will be documented as part documenting the SSL feature.
See [this comment](https://issues.apache.org/jira/browse/CELEBORN-1353?focusedCommentId=17837988&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17837988) in particular to ensure it is not missed.
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1574141536
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1371,12 +1397,24 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
object CelebornConf extends Logging {
- val TRANSPORT_MODULE_FALLBACKS = Map(
- "rpc_service" -> "rpc",
- "rpc_app" -> "rpc",
+ val TRANSPORT_MODULE_FALLBACKS: Map[String, String] = Map(
Review Comment:
Yes, these were added in previous PR btw.
See [here](https://github.com/apache/celeborn/blob/11c7d1080c76b86061b46ae3f99ceade37086f82/common/src/test/scala/org/apache/celeborn/common/CelebornConfSuite.scala#L379) for example.
In the current PR as well, the fallbacks are exercised in the tests - the configs are set as users would set them (so not for lifecycle manager, as that is internal), while the test itself would be for the internal module - and so the configs resolution would exercise fallback.
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#issuecomment-2069859439
Thanks for the reviews @SteNicholas, @waitinfuture !
--
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-1354 Followup: split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#issuecomment-2067967411
+CC @waitinfuture
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1574061723
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -24,11 +24,21 @@ public class TransportModuleConstants {
// RPC module used by the application components to communicate with each other
// This is used only at the application side.
+ // This is interally further split into RPC_LIFECYCLEMANAGER_MODULE and
+ // RPC_APP_CLIENT_MODULE - both of which inherit from RPC_APP_MODULE
+ // So for users, there is only RPC_APP_MODULE
public static final String RPC_APP_MODULE = "rpc_app";
// RPC module used to communicate with/between server components
// This is used both at server (master/worker) and application side.
public static final String RPC_SERVICE_MODULE = "rpc_service";
+ // See RPC_APP_MODULE for details - both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // are internal modules, and transport configs are not expected for these.
+ // For example, auto-ssl requires both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // to be in sync, though it is enabled only in RPC_LIFECYCLEMANAGER_MODULE
+ public static final String RPC_LIFECYCLEMANAGER_MODULE = "rpc_app_lifecyclemanager";
+ public static final String RPC_APP_CLIENT_MODULE = "rpc_app_client";
Review Comment:
@mridulm, IMO, we could distinguish which module or config option is internal in the configuration document. WDYT?
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1574141536
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1371,12 +1397,24 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
object CelebornConf extends Logging {
- val TRANSPORT_MODULE_FALLBACKS = Map(
- "rpc_service" -> "rpc",
- "rpc_app" -> "rpc",
+ val TRANSPORT_MODULE_FALLBACKS: Map[String, String] = Map(
Review Comment:
Yes, these were added in previous PR btw.
See [here](https://github.com/apache/celeborn/blob/11c7d1080c76b86061b46ae3f99ceade37086f82/common/src/test/scala/org/apache/celeborn/common/CelebornConfSuite.scala#L379) for example.
In the current PR as well, the fallbacks are exercised - the configs are set as users would set them (so not for lifecycle manager, as that is internal), while the test itself would be for the internal module - and so the configs resolution would exercise fallback.
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573950682
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -24,11 +24,21 @@ public class TransportModuleConstants {
// RPC module used by the application components to communicate with each other
// This is used only at the application side.
+ // This is interally further split into RPC_LIFECYCLEMANAGER_MODULE and
+ // RPC_APP_CLIENT_MODULE - both of which inherit from RPC_APP_MODULE
+ // So for users, there is only RPC_APP_MODULE
public static final String RPC_APP_MODULE = "rpc_app";
// RPC module used to communicate with/between server components
// This is used both at server (master/worker) and application side.
public static final String RPC_SERVICE_MODULE = "rpc_service";
+ // See RPC_APP_MODULE for details - both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // are internal modules, and transport configs are not expected for these.
+ // For example, auto-ssl requires both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // to be in sync, though it is enabled only in RPC_LIFECYCLEMANAGER_MODULE
+ public static final String RPC_LIFECYCLEMANAGER_MODULE = "rpc_app_lifecyclemanager";
+ public static final String RPC_APP_CLIENT_MODULE = "rpc_app_client";
Review Comment:
You are right - which is why this is not exposed to users, it is internal to Celeborn.
Please see the [comment here](https://github.com/apache/celeborn/pull/2471/files#diff-d5cb375c690ff45434efc9ca88e01ff6aa3122ed376656dd060512e955da745cR35) and [this warning ](https://github.com/apache/celeborn/pull/2471/files#diff-d6ae0fde704411e24d619d6034f2a00624c2e579b145fa6f10580c6c6a10adadR90) in case users do end up using it (by looking at the code).
Any suggestions to make this a better user experience ? To prevent confusion ?
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573950682
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -24,11 +24,21 @@ public class TransportModuleConstants {
// RPC module used by the application components to communicate with each other
// This is used only at the application side.
+ // This is interally further split into RPC_LIFECYCLEMANAGER_MODULE and
+ // RPC_APP_CLIENT_MODULE - both of which inherit from RPC_APP_MODULE
+ // So for users, there is only RPC_APP_MODULE
public static final String RPC_APP_MODULE = "rpc_app";
// RPC module used to communicate with/between server components
// This is used both at server (master/worker) and application side.
public static final String RPC_SERVICE_MODULE = "rpc_service";
+ // See RPC_APP_MODULE for details - both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // are internal modules, and transport configs are not expected for these.
+ // For example, auto-ssl requires both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // to be in sync, though it is enabled only in RPC_LIFECYCLEMANAGER_MODULE
+ public static final String RPC_LIFECYCLEMANAGER_MODULE = "rpc_app_lifecyclemanager";
+ public static final String RPC_APP_CLIENT_MODULE = "rpc_app_client";
Review Comment:
You are right - which is why this is not exposed to users, it is internal to Celeborn.
Please see the [comment here](https://github.com/apache/celeborn/pull/2471/files#diff-d5cb375c690ff45434efc9ca88e01ff6aa3122ed376656dd060512e955da745cR35) and [this warning ](https://github.com/apache/celeborn/pull/2471/files#diff-d6ae0fde704411e24d619d6034f2a00624c2e579b145fa6f10580c6c6a10adadR90) in case users do end up using it (by looking at the code).
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573950428
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -87,13 +87,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
this
}
+ private def warnIfInternalTransportModule(module: String, key: String): Unit = {
+ // for now, assert false to test
Review Comment:
Should remove it - I had an assertion here earlier :-)
--
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-1354 Followup: split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#issuecomment-2067972537
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/2471?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `77.77778%` with `10 lines` in your changes are missing coverage. Please review.
> Project coverage is 40.68%. Comparing base [(`121395f`)](https://app.codecov.io/gh/apache/celeborn/commit/121395f0f53656b21b22517e69fabc79cc4ef2ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`1350264`)](https://app.codecov.io/gh/apache/celeborn/pull/2471?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 8 commits behind head on main.
| [Files](https://app.codecov.io/gh/apache/celeborn/pull/2471?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/celeborn/pull/2471?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2FCelebornConf.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | 68.43% | [4 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../scala/org/apache/celeborn/common/util/Utils.scala](https://app.codecov.io/gh/apache/celeborn/pull/2471?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Futil%2FUtils.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3V0aWwvVXRpbHMuc2NhbGE=) | 66.67% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2471?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2471 +/- ##
==========================================
+ Coverage 40.17% 40.68% +0.51%
==========================================
Files 218 219 +1
Lines 13742 13903 +161
Branches 1214 1235 +21
==========================================
+ Hits 5520 5655 +135
- Misses 7905 7922 +17
- Partials 317 326 +9
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/2471?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1573838324
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -87,13 +87,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
this
}
+ private def warnIfInternalTransportModule(module: String, key: String): Unit = {
+ // for now, assert false to test
Review Comment:
Could this add the TODO comment for this?
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -24,11 +24,21 @@ public class TransportModuleConstants {
// RPC module used by the application components to communicate with each other
// This is used only at the application side.
+ // This is interally further split into RPC_LIFECYCLEMANAGER_MODULE and
+ // RPC_APP_CLIENT_MODULE - both of which inherit from RPC_APP_MODULE
+ // So for users, there is only RPC_APP_MODULE
public static final String RPC_APP_MODULE = "rpc_app";
// RPC module used to communicate with/between server components
// This is used both at server (master/worker) and application side.
public static final String RPC_SERVICE_MODULE = "rpc_service";
+ // See RPC_APP_MODULE for details - both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // are internal modules, and transport configs are not expected for these.
+ // For example, auto-ssl requires both RPC_LIFECYCLEMANAGER_MODULE and RPC_APP_CLIENT_MODULE
+ // to be in sync, though it is enabled only in RPC_LIFECYCLEMANAGER_MODULE
+ public static final String RPC_LIFECYCLEMANAGER_MODULE = "rpc_app_lifecyclemanager";
+ public static final String RPC_APP_CLIENT_MODULE = "rpc_app_client";
Review Comment:
What's the difference between `rpc_app_client` and `rpc_app` module? From a user perspective, this can easily be confusing.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1371,12 +1397,24 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
object CelebornConf extends Logging {
- val TRANSPORT_MODULE_FALLBACKS = Map(
- "rpc_service" -> "rpc",
- "rpc_app" -> "rpc",
+ val TRANSPORT_MODULE_FALLBACKS: Map[String, String] = Map(
Review Comment:
Would this fallback document later?
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2471:
URL: https://github.com/apache/celeborn/pull/2471#discussion_r1574126043
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1371,12 +1397,24 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
object CelebornConf extends Logging {
- val TRANSPORT_MODULE_FALLBACKS = Map(
- "rpc_service" -> "rpc",
- "rpc_app" -> "rpc",
+ val TRANSPORT_MODULE_FALLBACKS: Map[String, String] = Map(
Review Comment:
@mridulm, do the fallbacks have any test case to verify?
--
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-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client [celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2471: [CELEBORN-1354][FOLLOWUP] Split rpc_app into rpc_app_lifecyclemanager and rpc_app_client
URL: https://github.com/apache/celeborn/pull/2471
--
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