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

Allow for specifying extra class paths to embedded server. #5282

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

supernomad
Copy link
Contributor

This wires in the extra_classpath argument from start_jvm to the constructor for the Server instance. This will allow for users to easily write python unit tests that can interact with custom Java implementations as well.

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. I don't see this conflicting with #5275, or future ambitions of sourcing defaults from environment variables. Tagging @niloc132 for review.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good to me... any reason we should accept any of the other parameters?


        jvm_properties = None,
        java_home = None,
        extra_classpath = [],
        propfile: str = None,
        config = None

@supernomad
Copy link
Contributor Author

Looks good to me... any reason we should accept any of the other parameters?


        jvm_properties = None,
        java_home = None,
        extra_classpath = [],
        propfile: str = None,
        config = None

I am more than happy to add wiring for all of these if we want, won't take me long so let me know!

@devinrsmith
Copy link
Member

Looks good to me... any reason we should accept any of the other parameters?


        jvm_properties = None,
        java_home = None,
        extra_classpath = [],
        propfile: str = None,
        config = None

I might prefer to expose less for now to give ourselves the flexibility to change in the future if we need to. For example, I think setting an explicit propfile is bad practice, and we should prefer to inherit it based on the configuration dir.

Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@chipkent chipkent left a comment

Choose a reason for hiding this comment

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

LGTM

@devinrsmith devinrsmith merged commit 923823b into deephaven:main Mar 25, 2024
16 of 19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants