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

Move rlimit calls into NativeAccess #108805

Merged
merged 7 commits into from
May 20, 2024

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented May 18, 2024

This commit moves getting max threads, max virtual memory size, and max file size into NativeAccess.

relates #104876

This commit moves getting max threads, max virtual memory size, and max
file size into NativeAccess.

relates elastic#104876
@rjernst rjernst added :Core/Infra/Core Core issues without another label >refactoring labels May 18, 2024
@rjernst rjernst requested a review from a team as a code owner May 18, 2024 17:51
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label May 18, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@@ -16,10 +16,20 @@ class LinuxNativeAccess extends PosixNativeAccess {
Systemd systemd;

LinuxNativeAccess(NativeLibraryProvider libraryProvider) {
super("Linux", libraryProvider);
super("Linux", libraryProvider, new PosixConstants(-1L, 9, 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if it would be more readable to have those numbers as constants (like here https://github.com/elastic/elasticsearch/pull/108805/files#diff-e911107fc6225d34875cc491c2024e2eb415a4817a86d96b9b331cff63408da7R18 perhaps?
or at least with a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pretty readable in intellij (it shows the parameter name). I want to avoid creating individual static constants for each because they would just be passed through in this constants object, which is also statically held, meaning they would be in memory twice for no good reason.

@@ -326,10 +326,6 @@ public boolean handle(int code) {
// we've already logged this.
}

Natives.trySetMaxNumberOfThreads();
Copy link
Contributor

Choose a reason for hiding this comment

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

was this previously was 'caching' maxNumberOfThreads, MaxSizeVirtualMemory etc?
Do we want to do something similar with this refactoring?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously these try methods were called here, which set the values returned from rlimit in global statics in JNANatives. Now these values are retrieved during construction of NativeAccess instead of explicitly. By doing it during construction they do not need to be mutable as they were before.

@rjernst rjernst requested a review from a team May 20, 2024 13:07
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 20, 2024
@elasticsearchmachine elasticsearchmachine merged commit bc499e7 into elastic:main May 20, 2024
15 checks passed
@rjernst rjernst deleted the native/rlimit branch May 20, 2024 15:10
jedrazb pushed a commit to jedrazb/elasticsearch that referenced this pull request May 21, 2024
This commit moves getting max threads, max virtual memory size, and max
file size into NativeAccess.

relates elastic#104876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants