You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "onebox-li (via GitHub)" <gi...@apache.org> on 2023/05/15 10:31:24 UTC
[GitHub] [incubator-celeborn] onebox-li opened a new pull request, #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful …
onebox-li opened a new pull request, #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487
…shutdown is enabled
### What changes were proposed in this pull request?
When worker graceful shutdown is enabled and defined recoverPath `celeborn.worker.graceful.shutdown.recoverPath` does not exists, init leveldb will fail in `NativeDB.open`(caused by: IO error: /tmp/recover/recovery.ldb/LOCK: No such file or directory), and then recover function is not actually enabled.
We can create the recoverPath before init leveldb to avoid this error.
### Why are the changes needed?
Improve user experience
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Use the company test env
--
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
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1194664965
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,18 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
+ }
+ } catch {
+ case e: Exception =>
+ logError("Check or create recover root path failed: ", e)
Review Comment:
Yea
--
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
[GitHub] [incubator-celeborn] RexXiong merged pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong merged PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487
--
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
[GitHub] [incubator-celeborn] RexXiong commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1195878118
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,19 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
Small Suggestion: we can just change this to org.apache.commons.io.FileUtils.forceMkdir
--
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
[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1194655032
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,18 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
+ }
+ } catch {
+ case e: Exception =>
+ logError("Check or create recover root path failed: ", e)
Review Comment:
do you mean throw exception if failed, and let worker down?
--
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
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#issuecomment-1547880266
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1487?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1487](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1487?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (de0112d) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/5769c3fdc7eebd7701c40be364c83374ff7ea03b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5769c3f) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1487 +/- ##
==========================================
- Coverage 45.02% 45.01% -0.01%
==========================================
Files 158 158
Lines 9678 9678
Branches 957 957
==========================================
- Hits 4357 4356 -1
Misses 5034 5034
- Partials 287 288 +1
```
[see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1487/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
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1194529519
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,18 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
+ }
+ } catch {
+ case e: Exception =>
+ logError("Check or create recover root path failed: ", e)
Review Comment:
Sorry, forgot to add comment that here we still better to throw exception here.
--
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
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1195859633
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,19 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
Should we make this place support recursive mkdir? 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
[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1193667163
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,13 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
ok, i will update 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
[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1194511237
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,13 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
> We should support mkdir -P? and also have a try catch here?
I think mkdirs() is equivalent to mkdir -P
--
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
[GitHub] [incubator-celeborn] AngersZhuuuu commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful …
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1193661027
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,13 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
We should support mkdir -P? and also have a try catch here?
--
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
[GitHub] [incubator-celeborn] RexXiong commented on a diff in pull request #1487: [CELEBORN-585] Create if not exists worker recoverPath when graceful shutdown is enabled
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #1487:
URL: https://github.com/apache/incubator-celeborn/pull/1487#discussion_r1195888334
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -79,6 +80,19 @@ private[celeborn] class Worker(
conf.workerRpcPort != 0 && conf.workerFetchPort != 0 &&
conf.workerPushPort != 0 && conf.workerReplicatePort != 0),
"If enable graceful shutdown, the worker should use stable server port.")
+ if (gracefulShutdown) {
+ try {
+ val recoverRoot = new File(conf.workerRecoverPath)
+ if (!recoverRoot.exists()) {
+ logInfo(s"Recover root path ${conf.workerRecoverPath} does not exists, create it first.")
+ recoverRoot.mkdirs()
Review Comment:
> Should we make this place support recursive mkdir? cc @waitinfuture
Seems ok, I will merge it.
--
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