Skip to content
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

Allow affix settings to be dynamic / updatable #22526

Merged
merged 5 commits into from Jan 11, 2017

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Jan 10, 2017

Today affix settings are not dynamic since it's required to know
it's namespace in order to pull a concrete setting from it. This is not possible
in practice since the namespaces are dynamic by design. This change allows to register
a specialized settings consumer that consumes the namespace and the actual value if
a setting gets updated.

Allow affix settings to be dynamic / updateable
Today affix settings are not dynamic since it's required to know
it's namespace in order to pull a concrete setting from it. This is not possible
in practice since the namespaces are dynamic by design. This change allows to register
a specialized settings consumer that consumes the namespace and the actual value if
a setting gets updated.

@s1monw s1monw requested review from nik9000 and removed request for javanna Jan 10, 2017

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

@elasticmachine test this please

@nik9000
Copy link
Contributor

left a comment

LGTM. I left a few nit picks but nothing interesting. It'd be nice to have a non-test consumer for addAffixUpdateConsumer but isn't required for now.

@@ -449,6 +452,76 @@ public String toString() {
};
}

public static class AffixSetting<T> extends Setting<T> {

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Contributor

Probably worth javadocs.

/**
* Returns a string representation of the concrete setting key
*/
String getNamesapce(String key) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Contributor

s/Namesapce/Namespace/

public void testAddConsumerAffix() {
Setting.AffixSetting<Integer> testSetting = Setting.affixKeySetting("foo.", "bar",
(k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope));
Setting.AffixSetting<List<Integer>> testSetting2 = Setting.affixKeySetting("foo.", "list",

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Contributor

name it listSetting?

Map<String, List<Integer>> listResults = new HashMap<>();
Map<String, Integer> intResults = new HashMap<>();

BiConsumer<String, List<Integer>> listConsumer = listResults::put;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Contributor

I'd switch the order of these so it matches the declaration order.

.putArray("foo.test_list.list", "16", "17")
.putArray("foo.test_list_1.list", "18", "19", "20")
.build());
assertEquals(2, intResults.get("test").intValue());

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 10, 2017

Contributor

Very cute.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

LGTM. I left a few nit picks but nothing interesting. It'd be nice to have a non-test consumer for addAffixUpdateConsumer but isn't required for now.

cool thx, I will use this in #22502 but I didn't wanna make it part of that

s1monw added some commits Jan 10, 2017

@s1monw s1monw merged commit acb7f78 into elastic:master Jan 11, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:make_affix_updateable branch Jan 11, 2017

s1monw added a commit that referenced this pull request Jan 11, 2017

Allow affix settings to be dynamic / updatable (#22526)
Today affix settings are not dynamic since it's required to know
it's namespace in order to pull a concrete setting from it. This is not possible
in practice since the namespaces are dynamic by design. This change allows to register
a specialized settings consumer that consumes the namespace and the actual value if
a setting gets updated.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 11, 2017

Merge branch 'master' into remove-doc-links
* master:
  Prevent open channel leaks if handshake times out or is interrupted (elastic#22554)
  Remove SearchRequestParsers (elastic#22538)
  [TEST] Disable testRamBytesUsed on JDK 9
  [docs] Add missing RUN command from custom docker config
  Clean up SearchShardTarget (elastic#22468)
  Allow affix settings to be dynamic / updatable (elastic#22526)
  Remove ClusterService from ctors in reindex (elastic#22539)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.