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 534: Provide support to configure segment store port #535

Merged
merged 5 commits into from
May 6, 2021

Conversation

anishakj
Copy link
Contributor

@anishakj anishakj commented May 3, 2021

Signed-off-by: anishakj [email protected]

Change log description

Provided support to configure segment store ports. If the port is not mentioned in pravega options,default port will be set as 12345

Purpose of the change

Fixes #534

What the code does

Added support to configure segment store port.

How to verify it

Verified that segment store pods are coming up with different port number, if it is configured.
If port number is not specified, segment store pod is coming up with default port number
Verified changing segment store port at run time after Pravega is installed.
Verified changing segment store port at run time after Pravega is installed with external access.

@anishakj anishakj marked this pull request as ready for review May 3, 2021 18:16
@anishakj anishakj requested a review from SrishT May 3, 2021 18:16
Copy link

@gaddamas gaddamas left a comment

Choose a reason for hiding this comment

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

Overall this looks good, however when customizing port in single IP environment (multiple segmentstores exposed on single IP with different ports) customer should be aware they still need to open-up ports on firewall, since we take configured port and increment for each additional ss replica.

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #535 (d5320c6) into master (f3c81fb) will decrease coverage by 0.12%.
The diff coverage is 35.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   70.55%   70.43%   -0.13%     
==========================================
  Files          15       15              
  Lines        3573     3616      +43     
==========================================
+ Hits         2521     2547      +26     
- Misses        932      942      +10     
- Partials      120      127       +7     
Impacted Files Coverage Δ
...roller/pravegacluster/pravegacluster_controller.go 51.88% <16.66%> (+0.24%) ⬆️
pkg/controller/pravega/pravega_segmentstore.go 91.72% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3c81fb...d5320c6. Read the comment docs.

@RaulGracia RaulGracia changed the title Issue 534: Provide support to configure segment sore port Issue 534: Provide support to configure segment store port May 5, 2021
Signed-off-by: anishakj <[email protected]>
if err != nil {
return fmt.Errorf("failed to obtain configmap: %v", err)
}
javaopts := cm.Data["JAVA_OPTS"]
Copy link
Contributor

Choose a reason for hiding this comment

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

there can also be a check to see if the configmap is nil and to check whether it contains the provided key or not. Also the key being passed can be another argument passed to the method to make it more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

anishakj added 2 commits May 6, 2021 01:00
Signed-off-by: anishakj <[email protected]>
Copy link
Contributor

@SrishT SrishT left a comment

Choose a reason for hiding this comment

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

LGTM

@anishakj anishakj merged commit 1e2c248 into master May 6, 2021
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.

Provide support to configure segment store port
4 participants