Issue 226: Pass jvm options #247

Tristan1900 · 2019-08-07T07:29:48Z

Signed-off-by: wenqimou [email protected]

Change log description

Pass jvm options to bookie, segmentstore and controller.

Purpose of the change

What the code does

Pass the jvm options, override the default if it exists already.
API change

bookkeeper:
  jvm:
    memoryOpts: []
    gcOpts: []
    gcLoggingOpts: []
    extraOpts: []

pravega:
  jvm:
    controller: []
    segmentstore: []

How to verify it

Unit tests should pass. The manual deployment should also see the options are passed to the jvm.

Signed-off-by: wenqimou <[email protected]>

andreipaduroiu · 2019-08-08T22:52:27Z

Does this also make them configurable? I only see hardcoded values.

The idea of this fix/PR is to make JVM options configurable from manifest. The operator would still add JVM options it is adding today, but users should be able to override these or add new values they may need.

Tristan1900 · 2019-08-09T16:23:03Z

Thanks for the review! @andreipaduroiu Yes, it is configurable. There is a method OverrideDefaultJVMOptions to override the default values or append the new values. We just need to call that method for the parameters from the manifest, and it will generate the desired set of parameters. There is also a set of unit tests to verify this method.

andreipaduroiu

LGTM. Thanks for the clarification.

Signed-off-by: wenqimou <[email protected]>

codecov-io · 2019-08-09T22:23:14Z

Codecov Report

Merging #247 into master will decrease coverage by 1.65%.
The diff coverage is 8.43%.

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
- Coverage   32.62%   30.96%   -1.66%     
==========================================
  Files           9        9              
  Lines        1128     1211      +83     
==========================================
+ Hits          368      375       +7     
- Misses        712      788      +76     
  Partials       48       48

Impacted Files	Coverage Δ
pkg/apis/pravega/v1alpha1/zz_generated.deepcopy.go	`0% <0%> (ø)`	⬆️
pkg/apis/pravega/v1alpha1/bookkeeper.go	`100% <100%> (ø)`	⬆️
pkg/apis/pravega/v1alpha1/pravega.go	`100% <100%> (ø)`	⬆️

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 0f1a793...732a851. Read the comment docs.

fpj · 2019-08-11T14:58:13Z

The description says Fix #226, but the title says #246. Is #246 a duplicate of #226?

Tristan1900 · 2019-08-12T00:52:44Z

@fpj Thanks for the notice. I opened a dup issue before. Fixing the name.

Signed-off-by: wenqimou <[email protected]>

RaulGracia

LGTM. It would be nice to have the default JVM values in the example yaml (cr-detailed.yaml) so people have a template of how to modify these values. Once you address this, I will approve. Note that this PR is a requirement for pravega/pravega#4074, so let's see if we can get it merged soon.

RaulGracia · 2019-09-09T18:24:29Z

@Tristan1900 it would be nice to merge this PR soon. Could you please address my prior comment regarding adding and "example" of how to modify these JVM values in cr-detailed.yaml? I will approve after you add this. Thanks!

Tristan1900 · 2019-09-09T18:40:39Z

@RaulGracia Oh sure. Doing that now

Could you please review it again? Thanks a lot!

Signed-off-by: wenqimou <[email protected]>

RaulGracia · 2019-09-12T17:15:47Z

@pbelgundi please, review this PR and merge it asap if there is no additional comments on it. Note that having the functionality of this PR is becoming a pressing issue, as the new streaming cache for Pravega depends on this.

andreipaduroiu · 2019-09-12T17:29:24Z

Looks like some tests are failing? @Tristan1900 would you mind taking a quick look and see if they're related?

Tristan1900 · 2019-09-12T23:59:57Z

@andreipaduroiu Thanks for the notice! I am debugging it.

Signed-off-by: wenqimou <[email protected]>

Tristan1900 · 2019-09-13T01:50:28Z

I just found that UnlockExperimentalVMOptions has to be specified in front of UseCGroupMemoryLimitForHeap

Signed-off-by: wenqimou <[email protected]>

pkg/apis/pravega/v1alpha1/bookkeeper.go

pkg/apis/pravega/v1alpha1/pravega.go

pbelgundi · 2019-09-13T05:43:10Z

pkg/util/pravegacluster.go

+	return fmt.Sprintf("-XX:%v=%v", k, v)
+}
+
+func OverrideDefaultJVMOptions(defaultOpts []string, overrideOpts []string) []string {


I find this method way too complicated.....

Since the defaultOptions are in code, we could store them in a map instead of a slice like this:

defaultOptsMap := map[string]string { "-Xms":"512m", "-XX:+ExitOnOutOfMemoryError":"", "-XX:+CrashOnOutOfMemoryError",:"", "-XX:+HeapDumpOnOutOfMemoryError": "", "-XX:HeapDumpPath=": heapDumpDir, } )

For Overriding default values do the following:

mergedSlice = Create a new slice to holdall options [ overridden defaults + new user provided options + non-overridden default options ]

for key, value := range defaultOptsMap {
if (key is a prefix for any value in overrideSlice) {
// do nothing, since we want to pick this value from overrideSlice
} else {
// add default value that is not present in overrideSlice
mergedSlice = append(mergedSlice, key+value)
}

return mergedSliced.

Thanks for the comment. I see your point and here is what I think,

If the number of default options is m and the number of user-provided options is n, this line if (key is a prefix for any value in overrideSlice) will basically make the time complexity to be m*n, whereas my current way is m+n.

Since iteration for map in Go does not follow the insertion order, this line for key, value := range defaultOptsMap will make the iteration in random order. However, I found that we have to specify UnlockExperimentalVMOptions before UseCGroupMemoryLimitForHeap when passing to the JVM, otherwise JVM will throw an error. So there is an order that needs to be guaranteed when passing those JVM options.

I don't think we need to worry about execution complexity here, unless we expect the number of options ( default or user provided) to be > 100. In most cases the number of options provided ( default + user provided) should be not more than 10-15 ( At max this can go upto 30-35) which is too small a number to worry about execution complexity.
It is more important to have clear and readable code that is easier to maintain and reason about.
As regards the order of options, only a few options should be order sensitive and it should be easy to just add them first.

I've added some comments to make it more readable. It would be great if you could take a look at it again and see if it works. Thanks!

example/cr-detailed.yaml

Signed-off-by: wenqimou <[email protected]>

pbelgundi · 2019-09-16T07:42:53Z

pkg/util/pravegacluster.go

+	return fmt.Sprintf("-XX:%v=%v", k, v)
+}
+
+func OverrideDefaultJVMOptions(defaultOpts []string, overrideOpts []string) []string {


I don't think we need to worry about execution complexity here, unless we expect the number of options ( default or user provided) to be > 100. In most cases the number of options provided ( default + user provided) should be not more than 10-15 ( At max this can go upto 30-35) which is too small a number to worry about execution complexity.
It is more important to have clear and readable code that is easier to maintain and reason about.
As regards the order of options, only a few options should be order sensitive and it should be easy to just add them first.

Signed-off-by: wenqimou <[email protected]>

Pass jvm options

dc03046

Signed-off-by: wenqimou <[email protected]>

Tristan1900 requested review from andreipaduroiu, RaulGracia and pbelgundi August 7, 2019 07:30

andreipaduroiu approved these changes Aug 9, 2019

View reviewed changes

andreipaduroiu assigned RaulGracia and pbelgundi Aug 9, 2019

Add extraopts

333b81e

Signed-off-by: wenqimou <[email protected]>

Tristan1900 changed the title ~~Issue 246: Pass jvm options~~ Issue 226: Pass jvm options Aug 12, 2019

merge master

ad0c313

Signed-off-by: wenqimou <[email protected]>

RaulGracia requested changes Sep 2, 2019

View reviewed changes

wenqimou added 2 commits September 9, 2019 11:49

Add example

732a851

Signed-off-by: wenqimou <[email protected]>

merge master

8a0b32d

Signed-off-by: wenqimou <[email protected]>

RaulGracia approved these changes Sep 10, 2019

View reviewed changes

Tristan1900 unassigned RaulGracia Sep 11, 2019

Add sequence to map

74823b1

Signed-off-by: wenqimou <[email protected]>

minor fix

7e6520b

Signed-off-by: wenqimou <[email protected]>

pbelgundi requested changes Sep 13, 2019

View reviewed changes

Address comments

6b04e76

Signed-off-by: wenqimou <[email protected]>

Address comments

a440c40

Signed-off-by: wenqimou <[email protected]>

pbelgundi requested changes Sep 16, 2019

View reviewed changes

Add comment

412b308

Signed-off-by: wenqimou <[email protected]>

pbelgundi approved these changes Sep 18, 2019

View reviewed changes

pbelgundi merged commit 656eaf1 into master Sep 18, 2019

Tristan1900 deleted the issue-226-pass-java-opts branch September 18, 2019 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 226: Pass jvm options #247

Issue 226: Pass jvm options #247

Tristan1900 commented Aug 7, 2019 •

edited

Loading

andreipaduroiu commented Aug 8, 2019 •

edited by pbelgundi

Loading

Tristan1900 commented Aug 9, 2019

andreipaduroiu left a comment

codecov-io commented Aug 9, 2019 •

edited

Loading

fpj commented Aug 11, 2019 •

edited

Loading

Tristan1900 commented Aug 12, 2019

RaulGracia left a comment

RaulGracia commented Sep 9, 2019

Tristan1900 commented Sep 9, 2019 •

edited

Loading

RaulGracia commented Sep 12, 2019

andreipaduroiu commented Sep 12, 2019

Tristan1900 commented Sep 12, 2019

Tristan1900 commented Sep 13, 2019

pbelgundi Sep 13, 2019

Tristan1900 Sep 16, 2019 •

edited

Loading

pbelgundi Sep 16, 2019 •

edited

Loading

Tristan1900 Sep 18, 2019

pbelgundi Sep 16, 2019 •

edited

Loading

Issue 226: Pass jvm options #247

Issue 226: Pass jvm options #247

Conversation

Tristan1900 commented Aug 7, 2019 • edited Loading

Change log description

Purpose of the change

What the code does

How to verify it

andreipaduroiu commented Aug 8, 2019 • edited by pbelgundi Loading

Tristan1900 commented Aug 9, 2019

andreipaduroiu left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 9, 2019 • edited Loading

Codecov Report

fpj commented Aug 11, 2019 • edited Loading

Tristan1900 commented Aug 12, 2019

RaulGracia left a comment

Choose a reason for hiding this comment

RaulGracia commented Sep 9, 2019

Tristan1900 commented Sep 9, 2019 • edited Loading

RaulGracia commented Sep 12, 2019

andreipaduroiu commented Sep 12, 2019

Tristan1900 commented Sep 12, 2019

Tristan1900 commented Sep 13, 2019

pbelgundi Sep 13, 2019

Choose a reason for hiding this comment

Tristan1900 Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

pbelgundi Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Tristan1900 Sep 18, 2019

Choose a reason for hiding this comment

pbelgundi Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Tristan1900 commented Aug 7, 2019 •

edited

Loading

andreipaduroiu commented Aug 8, 2019 •

edited by pbelgundi

Loading

codecov-io commented Aug 9, 2019 •

edited

Loading

fpj commented Aug 11, 2019 •

edited

Loading

Tristan1900 commented Sep 9, 2019 •

edited

Loading

Tristan1900 Sep 16, 2019 •

edited

Loading

pbelgundi Sep 16, 2019 •

edited

Loading

pbelgundi Sep 16, 2019 •

edited

Loading