Set default elasticsearch heap size to 2GB (Alternate PR) #5684
Conversation
| // Spaces are deliberate to allow user to define additional jvm options as elasticsearch resolves option files lexicographically | ||
| withClasspathResourceMapping( | ||
| "elasticsearch-default-memory-vm.options", | ||
| "/usr/share/elasticsearch/config/jvm.options.d/ elasticsearch-default-memory-vm.options", |
There was a problem hiding this comment.
JVM processes options files in lexicographic order.
The file name " elasticsearch-default-memory-vm.options" was the best way I could think of to add a file that takes the least precedent over other files in this folder. As the space character is allowed in linux filenames and appears before most other characters in utf-8 and ascii encoding. But I could be unaware of common file names that go before this name lexicographically?
There was a problem hiding this comment.
The approach sounds pragmatic, but why multiple spaces?
There was a problem hiding this comment.
Just an additional precaution. If a user names their file " hello-world.options" - their file will still take precedence. That being said, I could potentially reduce the number of spaces (maybe 3-4?) - I doubt many users will prefix their files with spaces.
There was a problem hiding this comment.
Let's just do 1 space here, I think the effort is good enough.
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for opening this PR as an alternative to #5398. Your explanation is very sound and seems a technology idiomatic approach to handling this.
For now, I also prefer to not add an additional public API.
I'd like to give @DKarim a bit of time to react to this, but so far, I think it might be a good idea to go ahead with this approach.
| // Spaces are deliberate to allow user to define additional jvm options as elasticsearch resolves option files lexicographically | ||
| withClasspathResourceMapping( | ||
| "elasticsearch-default-memory-vm.options", | ||
| "/usr/share/elasticsearch/config/jvm.options.d/ elasticsearch-default-memory-vm.options", |
There was a problem hiding this comment.
The approach sounds pragmatic, but why multiple spaces?
...elasticsearch/src/test/java/org/testcontainers/elasticsearch/ElasticsearchContainerTest.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review @kiview . 👍 Code review comments have been addressed. |
...les/elasticsearch/src/main/java/org/testcontainers/elasticsearch/ElasticsearchContainer.java
Outdated
Show resolved
Hide resolved
…search/ElasticsearchContainer.java
|
Thanks for the contribution @REslim30 and thanks for the initial PR and bug report @DKarim. |
Proposed fix for issue #5393. Thought I could help to address comments from @DKarim 's PR #5398 as it looks like he is busy (hope I am not intruding with this PR @DKarim). Had some additional improvements I wanted to make as well.
Rationale behind changes
Decided against the additional method that @DKarim introduced. I think it's better to introduce additional public api in a separate issue that way we can determine if there's a real demand for it otherwise we'd be adding extra complexity and maintenance burden for little gain.
Also opted for usage of the
/usr/share/elasticsearch/config/jvm.options.d/folder over theES_JAVA_OPTSenvironment variable. For a few reasons:ES_JAVA_OPTStoCLI_JAVA_OPTSon their docs https://www.elastic.co/guide/en/elasticsearch/reference/current/advanced-configuration.html. After testing, it seems like they have backwards compatibility forES_JAVA_OPTS- but usage of both at the same time is unpredictable./usr/share/elasticsearch/config/jvm.options.d/, thus if any user had memory defined in one of these files then the default would have overwritten it:https://www.elastic.co/guide/en/elasticsearch/reference/7.17/advanced-configuration.html#set-jvm-options