Forbid changing thread pool types #14367
Conversation
| @@ -86,6 +72,84 @@ | |||
| public static final String FETCH_SHARD_STORE = "fetch_shard_store"; | |||
| } | |||
|
|
|||
| public enum ThreadPoolType { | |||
There was a problem hiding this comment.
Can you add javadocs for this, I think a single line for each type that gives a nice description for anyone checking out the code (support peeps) that might not be familiar with java threadpool types would be super helpful
There was a problem hiding this comment.
Nevermind, we already have docs for it in the actual docs, so we can leave it out here.
|
Left some comments but I like this! The only thing I wonder, however, is how a new 2.1.0 node joining a 2.0.0 node that already has custom threadpool settings will behave, once it sees the new settings it will try to apply them but fail, so you'll end up with different nodes with different configurations. This seems kind of dangerous, what do you think about deprecate in 2.x and remove in 3.0 only? |
I'd be ok if the 2.1 nodes just failed to start entirely if the cluster has this configuration and huffily printed out a message to the logs "detected dangerous threadpool configuration. Please change blah blah blah." So long as the parameter can be changed without bouncing the whole cluster this is something people can work around I think. Folks set these configuration variables like this are setting them in they pre-prod environments too hopefully. So it shouldn't be a surprise during upgrade for most folks. |
+1 only if it's easy to implement the failing to start. Since these settings are previously dynamic, someone can change their config and then start the 2.1.0 node up again. |
|
@dakrone I pushed a refactoring cleanup in 32b06108eda10d48e3b767f719e3e3d25485353b and validated dynamic settings attempts in fb30fc2775a700757c1169fc083b9ce650d10633. Would you mind taking a look? |
@dakrone As @jpountz remarked in the original pull request (when the idea was only to forbid changing thread pool types to type "cached"), this is a very expert setting so there will probably be very little if any real-world breakage. I'm fine adding something clear to the |
|
I'd certainly prefer it complain loudly in the logs if it gets a now invalid setting but I'm fine if ES doesn't fail to start. Or if it does. Either way. I like the idea of putting this in the 2.x line somewhere (2.1, 2.2, whatever) because it makes things less likely to crash. |
It's easy, but it'd be a hack. Right now we are very lenient about failure to apply settings. We'd basically have to special case this setting and I'm not a fan of that.
The problem here is that today we still do not have way to delete persistent cluster settings. That gets us into situations like #3670 which is still waiting on #6732 to be addressed. In this case, someone could have these settings applied on a 2.0.x cluster, they will no longer apply on a 2.1.y cluster, will see warning messages in the logs, but not have a clean way to remove the setting. @nik9000 @jpountz @dakrone I'm starting to lean towards 3.0 only. |
Not worth it for a hack. Let's not do that then.
+1 for 3.0 only, this isn't so important we need to jump through tons of hoops to get it in for 2.1. I think we can remove the docs for setting it in 2.1 though, just to reduce the chance of the type being set. |
|
@dakrone Removed the the byte serialization of |
|
LGTM, thanks @jasontedor ! |
|
So maybe in this PR we should keep accepting the type parameter if the value is equal to the right threadpool type and leave a comment saying that this hack can be removed when #3670 is in? Otherwise I'm afraid that this might get forgotten and that we will get failed upgrades when releasing 3.0. |
@jpountz Agree. Done in 843a394ea40e7a41081c245e0be9fa42661dec2e. |
| String type = groupSettings.get(key).get("type"); | ||
| // TODO: the type equality check can be removed after #3760/#6732 are addressed | ||
| if (type != null && !THREAD_POOL_TYPES.get(key).getType().equals(type)) { | ||
| throw new IllegalArgumentException("setting " + THREADPOOL_GROUP + key + ".type to " + type + " is not permitted"); |
There was a problem hiding this comment.
can the error message mention the only allowed value, so that users with existing settings can update?
There was a problem hiding this comment.
Yes, done in 87f3a6f118865547a677833210646481561eff7b.
|
LGTM. It should be backportable to 2.x under the current form, right? (not especially pushing for this, just wondering as what seemed to be the major concern against backporting to 2.x does not seem to apply anymore?) |
Yes.
Correct. |
…-types Forbid changing thread pool types
|
@jasontedor I'm not seeing the commit on the 2.1/2.x branches, do I miss anything? |
|
@jpountz It was a messy integration into those branches (from among other things, Guava still being used, lambdas not able to be used); I have it ready locally and will be getting it into origin soon. |
|
Thank you! |
This commit forbids the changing of thread pool types for any thread
pool. The motivation here is that these are expert settings with
little practical advantage.
Closes #14294, relates #2509, relates #2858, relates #5152