Add infrastructure to transactionally apply and reset dynamic settings by s1monw · Pull Request #15278 · elastic/elasticsearch

s1monw · 2015-12-07T11:50:39Z

This commit adds the infrastructure to make settings that are updateable
resetable and changes the application of updates to be transactional. This means
setting updates are either applied or not. If the application failes all values are rejected.

This initial commit converts all dynamic cluster settings to make use of the new infrastructure.
All cluster level dynamic settings are not resettable to their defaults or to the node level settings.
The infrastructure also allows to list default values and descriptions which is not fully implemented yet.

Values can be reset using a list of key or simple regular expressions. This has only been implemented on the java
layer yet. For instance to reset all recovery settings to their defaults a user can just specify indices.recovery.*.

This commit also adds strict settings validation, if a setting is unknown or if a setting can not be applied the entire
settings update request will fail.

s1monw · 2015-12-07T13:45:44Z

this issue basically closes #7704 and is a first cut at #6732

nik9000 · 2015-12-07T21:25:22Z

.../main/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequest.java

This commit adds the infrastructure to make settings that are updateable resetable and changes the application of updates to be transactional. This means setting updates are either applied or not. If the application failes all values are rejected. This initial commit converts all dynamic cluster settings to make use of the new infrastructure. All cluster level dynamic settings are not resettable to their defaults or to the node level settings. The infrastructure also allows to list default values and descriptions which is not fully implemented yet. Values can be reset using a list of key or simple regular expressions. This has only been implemented on the java layer yet. For instance to reset all recovery settings to their defaults a user can just specify `indices.recovery.*`. This commit also adds strict settings validation, if a setting is unknown or if a setting can not be applied the entire settings update request will fail.

nik9000 · 2015-12-08T14:47:48Z

Briefly looked at it last night but I'll actually review it now.

nik9000 · 2015-12-08T14:52:42Z

core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java

Since I haven't looked at setting the cluster readonly before I kind of assumed this would be handled in a listener in some ClusterStateService-style object rather than in settings. Maybe a comment why its here?

I see. This was extracted from ClusterSettingsService. It makes sense now. Cool.

s1monw · 2015-12-15T15:40:30Z

@bleskes pushed a new commit and merge with master

bleskes · 2015-12-16T09:12:17Z

...n/java/org/elasticsearch/cluster/routing/allocation/decider/ThrottlingAllocationDecider.java

imo the min value for these should be 1 - there are enough ways to block recovery that are more straight forward

I'd like to keep this as is for now - I think this needs a sep issue

sure. as long as we don't forget :)

bleskes · 2015-12-16T09:34:43Z

LGTM except for the array setting support. I'm fine with doing this as a followup as it seems we only have one dynamic cluster level setting that relies on that and that one can live just fine with the comma based. Note though that the array notation is used in more places which are non-dynamic (discovery.zen.ping.unicast.hosts, for example). I think we should do those as well (as a followup)?

s1monw · 2015-12-16T11:26:29Z

@bleskes I added yet another special case for setting which pisses me off but I have no choice. this has been fucked up in the past and is hard to fix for the future. Anyway I think it's ready and I will push very soon

bleskes · 2015-12-16T12:27:46Z

@s1monw thanks for the extra effort. Sadly, I don't think this is enough as we still don't deal correctly with how we parse json arrays into key.0 , key.1 etc. The request I gave above still fails and we move none dynamic settings into this infra we will have similar options. As I said before - I think we should push this in as iterate on this. We do need to support discovery.zen.ping.unicast.hosts: [ ... ] and network.bind_host: [ ... ] etc.

s1monw · 2015-12-16T13:17:15Z

@bleskes ok I added several tests that this works :)

bleskes · 2015-12-16T14:14:19Z

@s1monw thanks. I dug deeper as it still didn't work. Here's the problem, I think:

Index: core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java    (date 1450271602000)
+++ core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java    (revision )
@@ -43,7 +43,7 @@
             if (entry.getScope() != scope) {
                 throw new IllegalArgumentException("Setting must be a cluster setting but was: " + entry.getScope());
             }
-            if (entry.isGroupSetting()) {
+            if (entry.hasComplexMatcher()) {
                 complexMatchers.put(entry.getKey(), entry);
             } else {
                 keySettings.put(entry.getKey(), entry);

It seems we need one more test?

s1monw · 2015-12-16T19:18:05Z

more tests tests and tests I push this now it's not going to be better.

Add infrastructure to transactionally apply and reset dynamic settings

derjohn · 2016-01-12T16:19:25Z

\o/\o/\o/ THX !

s1monw added review release highlight :Core/Infra/Settings Settings infrastructure and APIs v5.0.0-alpha1 labels Dec 7, 2015

s1monw mentioned this pull request Dec 7, 2015

add support for resetting dynamic cluster settings #7704

Closed

nik9000 reviewed Dec 7, 2015
View reviewed changes

.../main/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequest.java Outdated

Copy link
Copy Markdown

Member

nik9000 Dec 7, 2015

Choose a reason for hiding this comment

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

leftover?

s1monw added 6 commits December 8, 2015 14:39

add rest API to reset settings

2e27ee3

fuck you linefeed

8502926

fix imports

44df681

expose default settings on /_cluster/settings?defaults=true

dfb8bf6

include default settigns

5d7f4ef

s1monw force-pushed the settings_prototype branch from 720fcb6 to 5d7f4ef Compare December 8, 2015 14:27

nik9000 reviewed Dec 8, 2015
View reviewed changes

bleskes reviewed Dec 16, 2015
View reviewed changes

s1monw added 4 commits December 16, 2015 10:38

allow -1 for concurrent rebalance, it's a special value :(

d0dbfce

fix list/array settings

9cf2f42

fix more excpption useage

32881c0

fix several .getKey usages

e586c96

fix cluster settings to accept complex list settigns

c1ff619

s1monw added 2 commits December 16, 2015 20:06

more tests

45b7afe

organize tests

161cabb

s1monw added 7 commits December 16, 2015 20:29

Merge branch 'master' into settings_prototype

71b204e

Convert several more settings

8543d77

Merge branch 'master' into settings_prototype

eae3da3

make it updateable

87f800c

Convert transport.tcp.compress setting

711f949

Merge branch 'master' into settings_prototype

eca2435

[TEST] use new settings infra

1862421

s1monw added a commit that referenced this pull request Dec 18, 2015

Merge pull request #15278 from s1monw/settings_prototype

55f77db

Add infrastructure to transactionally apply and reset dynamic settings

s1monw merged commit 55f77db into elastic:master Dec 18, 2015

s1monw deleted the settings_prototype branch December 18, 2015 09:57

clintongormley added the >feature label Jan 10, 2016

clintongormley mentioned this pull request Jan 12, 2016

Wrong "index.routing.allocation.enable" value prevents all allocation #15920

Closed

clintongormley mentioned this pull request Jan 18, 2016

Unable To Enable Writes Due To ClusterBlockException #11593

Closed

nachogiljaldo mentioned this pull request Aug 31, 2017

Discrepancy between 5.x documentation and actual behaviour on global state restore #26453

Closed

Conversation

s1monw commented Dec 7, 2015

Uh oh!

s1monw commented Dec 7, 2015

Uh oh!

nik9000 Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 8, 2015

Uh oh!

nik9000 Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

nik9000 Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

s1monw commented Dec 15, 2015

Uh oh!

bleskes Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

s1monw Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

s1monw Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

bleskes Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

bleskes commented Dec 16, 2015

Uh oh!

s1monw commented Dec 16, 2015

Uh oh!

bleskes commented Dec 16, 2015

Uh oh!

s1monw commented Dec 16, 2015

Uh oh!

bleskes commented Dec 16, 2015

Uh oh!

s1monw commented Dec 16, 2015

Uh oh!

derjohn commented Jan 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants