You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "FMX (via GitHub)" <gi...@apache.org> on 2023/10/10 07:22:46 UTC
[PR] [CELEBORN-1021] Celeborn support arbitary ratis configs and client rpc timeout [incubator-celeborn]
FMX opened a new pull request, #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969
### What changes were proposed in this pull request?
1. To support arbitrary ratis configs
2. To support Ratis client rpc timeout
### Why are the changes needed?
After some digs that I found out Celeborn never changed the default config of ratis client's timeout.
### Does this PR introduce _any_ user-facing change?
NO.
### How was this patch tested?
GA and cluster.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1359205642
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1776,6 +1781,22 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.SECONDS)
.createWithDefaultString("5s")
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
Review Comment:
Could this add method to parse the config of Ratis itself, which could not alway follow the Ratis config change?
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1352499055
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1765,7 +1770,7 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("3s")
+ .createWithDefaultString("30s")
Review Comment:
IMO the timeout seconds still large that may hide problems，how about (10s,15s) for default settings.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1765,7 +1770,7 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("3s")
+ .createWithDefaultString("30s")
Review Comment:
IMO the timeout seconds still large that may hide problems，how about (10s,15s) for default settings.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1351871769
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1774,7 +1779,23 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("5s")
+ .createWithDefaultString("45s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("30s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_WATCH_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.watch.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("45s")
Review Comment:
Looks larger than the original default value.
ratis client default config
```properties
raft.client.rpc.request.timeout=3000ms
raft.client.rpc..watch.request.timeout=10000ms
```
https://github.com/apache/ratis/blob/2e4e75bdc9671d9ac03eae50ce02ff4aa48ced1f/ratis-client/src/main/java/org/apache/ratis/client/RaftClientConfigKeys.java#L44-L56
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1774,7 +1779,23 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("5s")
+ .createWithDefaultString("45s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("30s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_WATCH_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.watch.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("45s")
Review Comment:
Looks larger than the original default value.
ratis client default config
```properties
raft.client.rpc.request.timeout=3000ms
raft.client.rpc.watch.request.timeout=10000ms
```
https://github.com/apache/ratis/blob/2e4e75bdc9671d9ac03eae50ce02ff4aa48ced1f/ratis-client/src/main/java/org/apache/ratis/client/RaftClientConfigKeys.java#L44-L56
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1352348639
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1774,7 +1779,23 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("5s")
+ .createWithDefaultString("45s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("30s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_WATCH_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.watch.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("45s")
Review Comment:
OK, docs will be updated in this PR.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1356772059
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1765,7 +1770,7 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("3s")
+ .createWithDefaultString("30s")
Review Comment:
I think the difference between 10 and 15 is smaller than the default value's difference. I think (10,20) might be a better choice.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1352295897
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1774,7 +1779,23 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("5s")
+ .createWithDefaultString("45s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("30s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_WATCH_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.watch.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("45s")
Review Comment:
Yes, setting this config larger than the default value is on purpose.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#issuecomment-1754652105
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1969?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1969](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1969?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e24ac46) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/56276e910f3489bc070846a19813e322b590fd17?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (56276e9) will **increase** coverage by `0.24%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## main #1969 +/- ##
==========================================
+ Coverage 46.96% 47.19% +0.24%
==========================================
Files 164 164
Lines 10362 10377 +15
Branches 957 957
==========================================
+ Hits 4865 4896 +31
+ Misses 5179 5166 -13
+ Partials 318 315 -3
```
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1969?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/1969?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | `87.60% <100.00%> (+0.08%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1969/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1361466074
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1776,6 +1781,22 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.SECONDS)
.createWithDefaultString("5s")
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
Review Comment:
Yes, there is a way to allow users who are familiar with Ratis to pass all their own Ratis configs to Celeborn.
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #1969: [CELEBORN-1021] Celeborn support arbitary Ratis configs and client rpc timeout
URL: https://github.com/apache/incubator-celeborn/pull/1969
--
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-1021] Celeborn support arbitary Ratis configs and client rpc timeout [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1969:
URL: https://github.com/apache/incubator-celeborn/pull/1969#discussion_r1352331721
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1774,7 +1779,23 @@ object CelebornConf extends Logging {
.categories("ha")
.version("0.3.0")
.timeConf(TimeUnit.SECONDS)
- .createWithDefaultString("5s")
+ .createWithDefaultString("45s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("30s")
+
+ val HA_MASTER_RATIS_CLIENT_RPC_WATCH_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.master.ha.ratis.raft.client.rpc.watch.timeout")
+ .internal
+ .categories("ha")
+ .version("0.3.2")
+ .timeConf(TimeUnit.SECONDS)
+ .createWithDefaultString("45s")
Review Comment:
Better to update the migration doc from 0.3.1 to 0.3.2 since the default value changed
--
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