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

Unify prop files for builds and production #1752

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

niloc132
Copy link
Member

Fixes #1732

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.

Happy with structural changes. Happy to change defaults, as long as we are aware of what's changing.

configs/dh-defaults.prop Show resolved Hide resolved
configs/dh-defaults.prop Show resolved Hide resolved
configs/dh-defaults.prop Show resolved Hide resolved
Comment on lines +17 to +19
NIO.driver.initialThreadCount=4
NIO.driver.maxThreadCount=16
Copy link
Member

Choose a reason for hiding this comment

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

Remind me, what happens at exhaustion? Do we throw exception, or schedule for next available thread.

Copy link
Member

Choose a reason for hiding this comment

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

We shutdown the process.

Copy link
Member

Choose a reason for hiding this comment

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

That said, we shouldn't be using this thread pool much anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was included in grpc-api.prop to begin with because the server wouldnt start without it. Startup today without that:

Initiating shutdown due to: Uncaught exception in thread UpdateGraphProcessor.DEFAULT.refreshThread
io.deephaven.configuration.PropertyException: Missing property in config file: NIO.driver.maxThreadCount
        at io.deephaven.configuration.PropertyFile.getProperty(PropertyFile.java:115)
        at io.deephaven.configuration.PropertyFile.getInteger(PropertyFile.java:126)
        at io.deephaven.net.impl.nio.NIODriver.init(NIODriver.java:106)
        at io.deephaven.net.impl.nio.NIODriver.init(NIODriver.java:96)
        at io.deephaven.net.CommBase.getScheduler(CommBase.java:68)
        at io.deephaven.engine.updategraph.UpdateGraphProcessor.refreshTablesAndFlushNotifications(UpdateGraphProcessor.java:1457)
        at io.deephaven.engine.updategraph.UpdateGraphProcessor$1.run(UpdateGraphProcessor.java:208)

@niloc132
Copy link
Member Author

niloc132 commented Dec 29, 2021

So, the important thing here is that dh-defaults.prop was only used for some tests and some codegen, but was never used at runtime by our running containers.

I have re-run all generators and there is no changes as a result of this to our .java or .py files.

The py tests were inconsistent before and some had broken paths, which is what prompted this change to begin with.

My assumption is that since workers have always* used the grpc-api.prop file's contents and we've never used dh-defaults.prop in any engine unit test or running container, that we would prefer the contents of grpc-api.prop, though we like the name of dh-defaults.prop better.

* since we ran a grpc server at all

@devinrsmith devinrsmith self-requested a review December 29, 2021 16:14
devinrsmith
devinrsmith previously approved these changes Dec 29, 2021
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.

Ah - I missed the part where you were taking the existing values from grpc-api.prop.

I think our defaults are due for an audit, but no reason to hold up this good structural change.

Comment on lines +17 to +19
NIO.driver.initialThreadCount=4
NIO.driver.maxThreadCount=16
Copy link
Member

Choose a reason for hiding this comment

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

That said, we shouldn't be using this thread pool much anymore.

configs/dh-defaults.prop Show resolved Hide resolved
configs/dh-defaults.prop Show resolved Hide resolved
configs/dh-defaults.prop Outdated Show resolved Hide resolved
configs/dh-defaults.prop Show resolved Hide resolved
@niloc132 niloc132 merged commit 428d7d7 into deephaven:main Jan 3, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2022
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.

dh-defaults.prop either needs to be replaced by grpc-api.prop or cleaned up for DHC
3 participants