-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Set default elasticsearch heap size to 2GB (Alternate PR) #5684
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach sounds pragmatic, but why multiple spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_OPTS
environment variable. For a few reasons:ES_JAVA_OPTS
toCLI_JAVA_OPTS
on 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