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 ClusterState.Custom to be created on initial cluster states #26144

Merged
merged 5 commits into from Aug 11, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Aug 10, 2017

Today we have a null invariant on all ClusterState.Custom. This makes
several code paths complicated and requires complex state handling in some cases.
This change allows to register a custom supplier that is used to initialize the
initial clusterstate with these transient customs.

Today we have a `null` invariant on all `ClusterState.Custom`. This makes
several code paths complicated and requires complex state handling in some cases.
This change allows to register a custom supplier that is used to initialize the
initial clusterstate with these transient customs.
@s1monw
Copy link
Contributor Author

s1monw commented Aug 10, 2017

@bleskes @ywelsch FYI

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM. I just left a few naming suggestions.

/**
* Creates a new cluster state builder that is initialized with the cluster name and all initial cluster state customs.
*/
ClusterState.Builder newClusterStateBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe initalClusterStateBuilder? It's a bit confusing since this "new" is very different from the "new" in the previous method.

}

public static Map<String, Supplier<ClusterState.Custom>> getCustomSuppliers(List<ClusterPlugin> clusterPlugins) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to do the same for MetaData.Custom, so it might be a good idea to reflect that this is ClusterStateCustom and not MetaDataCustom in the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and I agree the supplier approach here makes more sense because it will make rolling cluster restart handling later easier.

* This allows custom clusterstate extensions to be always present and prevents invariants where clusterstates are published
* but customs are not initialized.
*/
default Map<String, Supplier<ClusterState.Custom>> getInitialCustomSupplier() { return Collections.emptyMap(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, we might want to add ClusterState to the name so we can have MetaData custom later.

Most of other things that deal with customs are in the Plugin itself: getNamedWriteables, getNamedWriteables, getCustomMetaDataUpgrader, etc. So, the Plugin interface might be a better place for it... or maybe we should move upgraders here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this plugin is the right place for it. I agree we should move others here.

@colings86 colings86 added v5.6.1 and removed v5.6.0 labels Aug 11, 2017
@s1monw s1monw merged commit 6f82b0c into elastic:master Aug 11, 2017
@s1monw s1monw deleted the add_initial_state_processor branch August 11, 2017 07:51
s1monw added a commit that referenced this pull request Aug 11, 2017
…26144)

Today we have a `null` invariant on all `ClusterState.Custom`. This makes
several code paths complicated and requires complex state handling in some cases.
This change allows to register a custom supplier that is used to initialize the
initial clusterstate with these transient customs.
s1monw added a commit that referenced this pull request Aug 11, 2017
…26144)

Today we have a `null` invariant on all `ClusterState.Custom`. This makes
several code paths complicated and requires complex state handling in some cases.
This change allows to register a custom supplier that is used to initialize the
initial clusterstate with these transient customs.
@s1monw s1monw removed the v5.6.1 label Aug 11, 2017
@s1monw
Copy link
Contributor Author

s1monw commented Aug 11, 2017

@imotov I looked into backporting to 5.6 and it's not as straight forward as I wanted it to be. I think we just go with 6.0 upwards. followups can then happen on 7.0

@imotov
Copy link
Contributor

imotov commented Aug 11, 2017

@s1monw sounds good.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2017
* master: (30 commits)
  Rewrite range queries with open bounds to exists query (elastic#26160)
  Fix eclipse compilation problem (elastic#26170)
  Epoch millis and second formats parse float implicitly (Closes elastic#14641) (elastic#26119)
  fix SplitProcessor targetField test (elastic#26178)
  Fixed typo in README.textile (elastic#26168)
  Fix incorrect class name in deleteByQuery docs (elastic#26151)
  Move more token filters to analysis-common module
  reindex: automatically choose the number of slices (elastic#26030)
  Fix serialization of the `_all` field. (elastic#26143)
  percolator: Hint what clauses are important in a conjunction query based on fields
  Remove unused Netty-related settings (elastic#26161)
  Remove SimpleQueryStringIT#testPhraseQueryOnFieldWithNoPositions.
  Tests: reenable ShardReduceIT#testIpRange.
  Allow `ClusterState.Custom` to be created on initial cluster states (elastic#26144)
  Teach the build about betas and rcs (elastic#26066)
  Fix wrong header level
  inner hits: Unfiltered nested source should keep its full path
  Document how to import Lucene Snapshot libs when elasticsearch clients (elastic#26113)
  Use `global_ordinals_hash` execution mode when sorting by sub aggregations. (elastic#26014)
  Make the README use a single type in examples. (elastic#26098)
  ...
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
ywelsch added a commit that referenced this pull request Aug 2, 2018
This infrastructure was introduced in #26144 and made obsolete in #30743
@jpountz jpountz added >feature :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed >feature labels Jan 29, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >enhancement v6.0.0-beta2 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants