Skip to content

Commit f31fcb1

Browse files
committed
Notify affixMap settings when any under the registered prefix matches (#28317)
* Notify affixMap settings when any under the registered prefix matches Previously if an affixMap setting was registered, and then a completely different setting was applied, the affixMap update consumer would be notified with an empty map. This caused settings that were previously set to be unset in local state in a consumer that assumed it would only be called when the affixMap setting was changed. This commit changes the behavior so if a prefix `foo.` is registered, any setting under the prefix will have the update consumer notified if there are changes starting with `foo.`. Resolves #28316 * Add unit test * Address feedback
1 parent 1dc8cb0 commit f31fcb1

File tree

3 files changed

+104
-2
lines changed

3 files changed

+104
-2
lines changed

server/src/main/java/org/elasticsearch/common/settings/Setting.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,7 @@ AbstractScopedSettings.SettingUpdater<Map<String, T>> newAffixMapUpdater(Consume
597597

598598
@Override
599599
public boolean hasChanged(Settings current, Settings previous) {
600-
return Stream.concat(matchStream(current), matchStream(previous)).findAny().isPresent();
600+
return current.filter(k -> match(k)).equals(previous.filter(k -> match(k))) == false;
601601
}
602602

603603
@Override
@@ -612,7 +612,7 @@ public Map<String, T> getValue(Settings current, Settings previous) {
612612
if (updater.hasChanged(current, previous)) {
613613
// only the ones that have changed otherwise we might get too many updates
614614
// the hasChanged above checks only if there are any changes
615-
T value = updater.getValue(current, previous);
615+
T value = updater.getValue(current, previous);
616616
if ((omitDefaults && value.equals(concreteSetting.getDefault(current))) == false) {
617617
result.put(namespace, value);
618618
}

server/src/test/java/org/elasticsearch/cluster/allocation/FilteringAllocationIT.java

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,14 @@
2121

2222
import org.apache.logging.log4j.Logger;
2323
import org.elasticsearch.cluster.ClusterState;
24+
import org.elasticsearch.cluster.health.ClusterHealthStatus;
2425
import org.elasticsearch.cluster.routing.IndexRoutingTable;
2526
import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
2627
import org.elasticsearch.cluster.routing.ShardRouting;
28+
import org.elasticsearch.cluster.routing.ShardRoutingState;
2729
import org.elasticsearch.cluster.routing.allocation.decider.FilterAllocationDecider;
2830
import org.elasticsearch.cluster.routing.allocation.decider.ThrottlingAllocationDecider;
31+
import org.elasticsearch.common.Strings;
2932
import org.elasticsearch.common.logging.Loggers;
3033
import org.elasticsearch.common.settings.Setting;
3134
import org.elasticsearch.common.settings.Settings;
@@ -34,7 +37,9 @@
3437
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
3538
import org.elasticsearch.test.ESIntegTestCase.Scope;
3639

40+
import java.util.HashSet;
3741
import java.util.List;
42+
import java.util.Set;
3843

3944
import static org.hamcrest.Matchers.equalTo;
4045

@@ -156,5 +161,58 @@ public void testInvalidIPFilterClusterSettings() {
156161
.execute().actionGet());
157162
assertEquals("invalid IP address [192.168.1.1.] for [" + filterSetting.getKey() + ipKey + "]", e.getMessage());
158163
}
164+
165+
public void testTransientSettingsStillApplied() throws Exception {
166+
List<String> nodes = internalCluster().startNodes(6);
167+
Set<String> excludeNodes = new HashSet<>(nodes.subList(0, 3));
168+
Set<String> includeNodes = new HashSet<>(nodes.subList(3, 6));
169+
logger.info("--> exclude: [{}], include: [{}]",
170+
Strings.collectionToCommaDelimitedString(excludeNodes),
171+
Strings.collectionToCommaDelimitedString(includeNodes));
172+
ensureStableCluster(6);
173+
client().admin().indices().prepareCreate("test").get();
174+
ensureGreen("test");
175+
176+
Settings exclude = Settings.builder().put("cluster.routing.allocation.exclude._name",
177+
Strings.collectionToCommaDelimitedString(excludeNodes)).build();
178+
179+
logger.info("--> updating settings");
180+
client().admin().cluster().prepareUpdateSettings().setTransientSettings(exclude).get();
181+
182+
logger.info("--> waiting for relocation");
183+
waitForRelocation(ClusterHealthStatus.GREEN);
184+
185+
ClusterState state = client().admin().cluster().prepareState().get().getState();
186+
187+
for (ShardRouting shard : state.getRoutingTable().shardsWithState(ShardRoutingState.STARTED)) {
188+
String node = state.getRoutingNodes().node(shard.currentNodeId()).node().getName();
189+
logger.info("--> shard on {} - {}", node, shard);
190+
assertTrue("shard on " + node + " but should only be on the include node list: " +
191+
Strings.collectionToCommaDelimitedString(includeNodes),
192+
includeNodes.contains(node));
193+
}
194+
195+
Settings other = Settings.builder().put("cluster.info.update.interval", "45s").build();
196+
197+
logger.info("--> updating settings with random persistent setting");
198+
client().admin().cluster().prepareUpdateSettings()
199+
.setPersistentSettings(other).setTransientSettings(exclude).get();
200+
201+
logger.info("--> waiting for relocation");
202+
waitForRelocation(ClusterHealthStatus.GREEN);
203+
204+
state = client().admin().cluster().prepareState().get().getState();
205+
206+
// The transient settings still exist in the state
207+
assertThat(state.metaData().transientSettings(), equalTo(exclude));
208+
209+
for (ShardRouting shard : state.getRoutingTable().shardsWithState(ShardRoutingState.STARTED)) {
210+
String node = state.getRoutingNodes().node(shard.currentNodeId()).node().getName();
211+
logger.info("--> shard on {} - {}", node, shard);
212+
assertTrue("shard on " + node + " but should only be on the include node list: " +
213+
Strings.collectionToCommaDelimitedString(includeNodes),
214+
includeNodes.contains(node));
215+
}
216+
}
159217
}
160218

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,21 @@ public void testAddConsumerAffixMap() {
261261
assertEquals(2, listResults.size());
262262
assertEquals(2, intResults.size());
263263

264+
service.applySettings(Settings.builder()
265+
.put("foo.test.bar", 2)
266+
.put("foo.test_1.bar", 7)
267+
.putList("foo.test_list.list", "16", "17")
268+
.putList("foo.test_list_1.list", "18", "19", "20")
269+
.build());
270+
271+
assertEquals(2, intResults.get("test").intValue());
272+
assertEquals(7, intResults.get("test_1").intValue());
273+
assertEquals(Arrays.asList(16, 17), listResults.get("test_list"));
274+
assertEquals(Arrays.asList(18, 19, 20), listResults.get("test_list_1"));
275+
assertEquals(2, listResults.size());
276+
assertEquals(2, intResults.size());
277+
278+
264279
listResults.clear();
265280
intResults.clear();
266281

@@ -286,6 +301,35 @@ public void testAddConsumerAffixMap() {
286301

287302
}
288303

304+
public void testAffixMapConsumerNotCalledWithNull() {
305+
Setting.AffixSetting<Integer> prefixSetting = Setting.prefixKeySetting("eggplant.",
306+
(k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope));
307+
Setting.AffixSetting<Integer> otherSetting = Setting.prefixKeySetting("other.",
308+
(k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope));
309+
AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY,new HashSet<>(Arrays.asList(prefixSetting, otherSetting)));
310+
Map<String, Integer> affixResults = new HashMap<>();
311+
312+
Consumer<Map<String,Integer>> consumer = (map) -> {
313+
logger.info("--> consuming settings {}", map);
314+
affixResults.clear();
315+
affixResults.putAll(map);
316+
};
317+
service.addAffixMapUpdateConsumer(prefixSetting, consumer, (s, k) -> {}, randomBoolean());
318+
assertEquals(0, affixResults.size());
319+
service.applySettings(Settings.builder()
320+
.put("eggplant._name", 2)
321+
.build());
322+
assertThat(affixResults.size(), equalTo(1));
323+
assertThat(affixResults.get("_name"), equalTo(2));
324+
325+
service.applySettings(Settings.builder()
326+
.put("eggplant._name", 2)
327+
.put("other.thing", 3)
328+
.build());
329+
330+
assertThat(affixResults.get("_name"), equalTo(2));
331+
}
332+
289333
public void testApply() {
290334
Setting<Integer> testSetting = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope);
291335
Setting<Integer> testSetting2 = Setting.intSetting("foo.bar.baz", 1, Property.Dynamic, Property.NodeScope);

0 commit comments

Comments
 (0)