You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "pan3793 (via GitHub)" <gi...@apache.org> on 2023/06/21 10:55:15 UTC

[GitHub] [incubator-celeborn] pan3793 opened a new pull request, #1616: [CELEBORN-707][MASTER] Remove env CELEBORN_MASTER_HOST and CELEBORN_MASTER_PORT

pan3793 opened a new pull request, #1616:
URL: https://github.com/apache/incubator-celeborn/pull/1616

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     - Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
     - Be sure to keep the PR description updated to reflect all changes.
     - Please write your PR title to summarize what this PR proposes.
     - If possible, provide a concise example to reproduce the issue for a faster review.
   -->
   
   ### What changes were proposed in this pull request?
   
   Remove environment variables `CELEBORN_MASTER_HOST` and `CELEBORN_MASTER_PORT`, and makes `CELEBORN_LOCAL_HOSTNAME` takes effect on both master and worker.
   
   ### Why are the changes needed?
   
   There are many different ways to configure the master/worker host and port, which makes the thing complex and inconsistent.
   
   After this change, 
   
   #### master
   
   1. cli args `--host` `--port` takes the highest priority
   2. then lookup env `CELEBORN_LOCAL_HOSTNAME`
   3. things are different when HA enabled and disabled
     3.1. when HA is disabled, lookup configurations `celeborn.master.host` and `celeborn.master.port`
     3.2. when HA is enabled, each node needs to know the whole cluster info,
        ```
        celeborn.master.ha.node.1.host clb-1
        celeborn.master.ha.node.1.port 9097
        celeborn.master.ha.node.2.host clb-2
        celeborn.master.ha.node.2.port 9097
        celeborn.master.ha.node.3.host clb-3
        celeborn.master.ha.node.3.port 9097
        ```
        in addition, `celeborn.master.ha.node.id=1` can be used to indicate the node id, otherwise, the master will try to bind each host to match the node id.
   
   #### worker
   
   1. cli args `--host` `--port` takes the highest priority
   2. then lookup env `CELEBORN_LOCAL_HOSTNAME`
   
   things are simple than the master case because each worker is not required to know others.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes.
   
   ### How was this patch tested?
   
   Review.


-- 
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] pan3793 commented on pull request #1616: [CELEBORN-707][MASTER] Remove env CELEBORN_MASTER_HOST and CELEBORN_MASTER_PORT

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1616:
URL: https://github.com/apache/incubator-celeborn/pull/1616#issuecomment-1605819355

   The further work, I propose adding a new configuration `celeborn.bind.preferIpAddress`(any naming suggestion is welcome) to control whether to use IP or hostname when `CELEBORN_LOCAL_HOSTNAME` is absent. This is useful in K8s scenario, given currently Celeborn always uses IP for registering, as Celeborn is deployed as STS, the Pod name will be preserved but Pod IP changed on restarting, it's painful during the rolling upgrade, any restarted worker Pod will be treated as a brand new worker and lost all shuffle data, it's really painful.


-- 
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 #1616: [CELEBORN-707][MASTER] Remove env CELEBORN_MASTER_HOST and CELEBORN_MASTER_PORT

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1616:
URL: https://github.com/apache/incubator-celeborn/pull/1616#issuecomment-1600683629

   ## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1616?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#1616](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1616?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (404adc3) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/8194407558976f53ab33a6668bfbab1544c80119?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8194407) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   ```diff
   @@           Coverage Diff           @@
   ##             main    #1616   +/-   ##
   =======================================
     Coverage   45.79%   45.79%           
   =======================================
     Files         159      159           
     Lines        9943     9943           
     Branches      970      970           
   =======================================
     Hits         4552     4552           
     Misses       5091     5091           
     Partials      300      300           
   ```
   
   
   
   :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] waitinfuture closed pull request #1616: [CELEBORN-707][MASTER] Remove env CELEBORN_MASTER_HOST and CELEBORN_MASTER_PORT

Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1616: [CELEBORN-707][MASTER] Remove env CELEBORN_MASTER_HOST and CELEBORN_MASTER_PORT
URL: https://github.com/apache/incubator-celeborn/pull/1616


-- 
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