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

Deephaven-in-docker gradle wiring support for env vars #3536

Merged

Conversation

niloc132
Copy link
Member

Enables tests to control the environment variables of a running deephaven server in docker, to configure the jvm or deephaven itself.

This patch also modifies py/go/cpp tests to only ask for a max of 512mb of heap, to avoid issues running in docker desktop.

Fixes #3535

Enables tests to control the environment variables of a running
deephaven server in docker, to configure the jvm or deephaven itself.

This patch also modifies py/go/cpp tests to only ask for a max od 512mb
of heap, to avoid issues running in docker desktop.

Fixes deephaven#3535
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

LGTM.

As a general note: by default, JVM ergonomics optimizes for a different GC collector when the heap size is small. For the deephaven server, we are explicitly forcing non-ergonomic defaults via JAVA_OPTS=-XX:+UseG1GC -XX:MaxGCPauseMillis=100 -XX:+UseStringDeduplication. Not that I think there is pressing needs, but if we find ourselves needing to further optimize integration tests cases for small heap sizes, we may wish to override the server defaults and explicitly set JAVA_OPTS instead of START_OPTS.

@niloc132
Copy link
Member Author

Yeah, I deliberately set START_OPTS to let those settings continue to be used, as that is what our docker-compose example does as well, under the assumption that we probably want tests to be tweaking the server no more than is obvious from examples.

I considered briefly just offering a list for jvm args, maybe a map for system properties, but it seemed not-too-dirty to just directly expose all env vars so we could decide when to set JAVA_OPTS, EXTRA_CLASSPATH as well as and when we need them.

Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM

@niloc132 niloc132 merged commit c59f751 into deephaven:main Mar 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lower the JVM max memory limit for running the Python client tests
3 participants