-
Notifications
You must be signed in to change notification settings - Fork 79
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
Shareable executors #616
Shareable executors #616
Conversation
3c6ee49
to
a1e372a
Compare
5a4cac8
to
69e8474
Compare
@@ -70,7 +72,11 @@ private NettyExecutor(EventLoopGroup eventLoopGroup, | |||
} | |||
|
|||
public void shut() { | |||
eventLoopGroup.shutdownGracefully(); | |||
try { | |||
eventLoopGroup.shutdownGracefully(0, 0, TimeUnit.SECONDS).await(5000); |
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.
Should we wait here?
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.
Hi. It does wait for shutdownGracefully
to come back.
Or do you refer to the quietPeriod
setting? But we never really supported a graceful shutdown properly. Yet. I think we need to implement that on a "styx level".
The reason for immediate quiet and grace period is to avoid excessive waiting in functional test suite.
However I do think, going forward, we need to add timeout controls and a shutdown future to this API. Perhaps that should be a next increment, providing that the work is completed before the Styx 1.1 release. WDYT?
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.
The executors will only ever be closed when styx shuts. Therefore any shutdown future, etc doesn't really matter at the moment as the JVM is going to shut anyway. I would revisit this code if or once we'll allow consumers to shut injected executors.
try { | ||
eventLoopGroup.shutdownGracefully(0, 0, TimeUnit.SECONDS).await(5000); | ||
} catch (InterruptedException e) { | ||
throw new RuntimeException(e); |
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 is clearing the interrupted flag, we've discussed previously it might not be the best option.
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.
Thanks. A good catch :-)
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.
Fixed.
* | ||
* @return Styx service instance | ||
*/ | ||
NettyExecutor create(String name, JsonNode configuration); |
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.
Should we try not to not pass a JsonNode here? It seems we keep propagating our Yaml based config.
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 code follows the idiomatic fashion of creating styx objects from yaml/json nodes. Sadly the styx object model relies entirely on yaml and/or JsonNode based configuration.
I have created a a proof of concept that lifts this restriction. Check the details in PR: #609 (Remove old application router).
Till then, it needs to stay in this "idiomatic" form.
@@ -65,7 +66,7 @@ class YamlFileConfigurationServiceTest : FunSpec() { | |||
try { | |||
action(service) | |||
} finally { | |||
service.stop().join() | |||
service.stop().get(2, SECONDS) |
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 could throw several exceptions here... makes me think we might want to implement Autocloseable.
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.
You mean, for the StyxService
?
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.
Just a couple of minor suggestiions.
components/proxy/src/main/java/com/hotels/styx/startup/StyxServerComponents.java
Show resolved
Hide resolved
components/proxy/src/main/java/com/hotels/styx/routing/config/Builtins.java
Outdated
Show resolved
Hide resolved
Add default `Styx-Client-Global-Worker` executor. HostProxy: Add `executor` configuration attribute. OriginsConfigConverter: Use `Styx-Client-Global-Worker` by default.
28e0698
to
83ba726
Compare
Summary
This PR allows end users to declare named executors (or threads) and share them between different Styx objects.
The executors only work with the new
HostProxy
andHttpServer
objects. The oldproxy
andadmin
servers, and the old Styx Client, which are being phased out, still use the oldclientWorkerThreadsCount
,workerThreadsCount
, andbossThreadsCount
settings.Configuration
The executors are declared in the executors block at the top-level of Styx configuration file:
The executor can be shared by other objects:
The top level yaml configuration follows the schema:
optional("executors", map(routingObject())),
Where
routingObject
is the familiartype/config/tags
triple. TheNettyExecutor
object follows the schema:Styx is started with 3 "global" executors:
Styx-Client-Global-Worker
,StyxHttpServer-Global-Worker
,StyxHttpServer-Global-Boss
. The routing objects will use them unless some other are specified byexecutor
field. Consequently these names are reserved. Styx will always make sure that default global executors are available.You can override the default global executor settings by specifying that reserved name in the
executor
configuration. For example, the following configuration will change the thread names for global server worker thread:Add a new
executor
field to YamlFileConfigurationService configuration. To specify the executor for the createdHostProxy
objects. Whenexecutor
field is absent, it should use the default global executor.