-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
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.
Happy with structural changes. Happy to change defaults, as long as we are aware of what's changing.
NIO.driver.initialThreadCount=4 | ||
NIO.driver.maxThreadCount=16 |
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.
Remind me, what happens at exhaustion? Do we throw exception, or schedule for next available thread.
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.
We shutdown the process.
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.
That said, we shouldn't be using this thread pool much anymore.
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.
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)
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
|
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.
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.
NIO.driver.initialThreadCount=4 | ||
NIO.driver.maxThreadCount=16 |
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.
That said, we shouldn't be using this thread pool much anymore.
a3cf39c
to
93bd0e2
Compare
Fixes #1732