Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE-290] Make RpcNodePort and HttpNodePort optional #305

Merged

Conversation

amaliujia
Copy link
Contributor

What changes were proposed in this pull request?

This PR makes RPC_SERVER_PORT and JETTY_HTTP_PORT in RSS conf as optional by setting up default values into the config entries.

Why are the changes needed?

Feature.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

UT

@amaliujia
Copy link
Contributor Author

The design.md in fact mentions two values for each port config entry:

    # RpcNodePort represents the port required by the rpc protocol of the coordinators,
    # and the range is the same as the port range of the NodePort type service in kubernetes.
    # By default, we will deploy two coordinators to ensure active-active.
    rpcNodePort:
      - 30001
      - 30011
    # httpNodePort represents the port required by the http protocol of the coordinators,
    # and the range is the same as the port range of the NodePort type service in kubernetes.
    # By default, we will deploy two coordinators to ensure active-active.
    httpNodePort:
      - 30002
      - 30012

However I didn't see two usages in the codebase. Maybe I have missed something?

zuston
zuston previously approved these changes Nov 7, 2022
@jerqi
Copy link
Contributor

jerqi commented Nov 7, 2022

We only provide the default value in the configuration. https://github.com/apache/incubator-uniffle/blob/master/conf/server.conf and https://github.com/apache/incubator-uniffle/blob/master/conf/coordinator.conf

rss.rpc.server.port 19999
rss.jetty.http.port 19998

design.md is only the Kubernetes Operator design document.

@amaliujia
Copy link
Contributor Author

I see it better now. Made a change to have the default value in config entries match the values in the provided config file.

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2022

Codecov Report

Merging #305 (d63d191) into master (fbe3074) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #305   +/-   ##
=========================================
  Coverage     60.70%   60.70%           
  Complexity     1459     1459           
=========================================
  Files           180      180           
  Lines          9223     9223           
  Branches        886      886           
=========================================
  Hits           5599     5599           
  Misses         3325     3325           
  Partials        299      299           
Impacted Files Coverage Δ
.../org/apache/uniffle/common/config/RssBaseConf.java 93.42% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @amaliujia @zuston

@jerqi jerqi merged commit 8890ed6 into apache:master Nov 7, 2022
@jerqi
Copy link
Contributor

jerqi commented Nov 16, 2022

Do you want to join our Wechat group? You can scan the QR code
企业微信截图_eb8711db-34bf-4365-bdab-15ef2e3ea119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants