[ISSUE-290] Make RpcNodePort and HttpNodePort optional by amaliujia · Pull Request #305 · apache/uniffle · GitHub
Skip to content

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

Merged
roryqi merged 2 commits into
apache:masterfrom
amaliujia:Make_RpcNodePort_and_HttpNodePort_optional
Nov 7, 2022
Merged

[ISSUE-290] Make RpcNodePort and HttpNodePort optional#305
roryqi merged 2 commits into
apache:masterfrom
amaliujia:Make_RpcNodePort_and_HttpNodePort_optional

Conversation

@amaliujia

Copy link
Copy Markdown

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
Copy Markdown
Author

zuston
zuston previously approved these changes Nov 7, 2022
@roryqi

roryqi commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Nov 7, 2022

Copy link
Copy Markdown

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

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @amaliujia @zuston

@roryqi roryqi merged commit 8890ed6 into apache:master Nov 7, 2022
@roryqi

roryqi commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

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.

4 participants