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

Introduce formal role for remote cluster client #53924

Merged
merged 15 commits into from Mar 24, 2020

Conversation

jasontedor
Copy link
Member

This commit introduce a formal role for identifying nodes that are capable of making connections to remote clusters.

This commit introduce a formal role for identifying nodes that are
capable of making connections to remote clusters.
@jasontedor jasontedor added >enhancement :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v8.0.0 v7.7.0 labels Mar 21, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Distributed)

@jasontedor jasontedor added the :Core/Infra/Core Core issues without another label label Mar 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

One question

public static Set<DiscoveryNodeRole> BUILT_IN_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE);
public static Set<DiscoveryNodeRole> BUILT_IN_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE, REMOTE_CLUSTER_CLIENT_ROLE);

static Set<DiscoveryNodeRole> LEGACY_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be based on version? At what point would this get updated? Perhaps we could use a map of role to min version (or a predicate) and iterate over them checking each role when serializing against the version of the output stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this shouldn't be based on version. We have two things:

  • very old nodes before we made roles pluggable that only understand three roles: master, data, and ingest
  • new nodes after we made roles pluggable that understand the roles they are built with us, but also understand that other nodes might send them roles that they don't understand
  • for example, a 7.6.0 node today while it doesn't have a role built-in for remote_cluster_client, does have the ability to receive that role from a 7.7.0 node, and represent it accordingly
  • however, a 7.0.0 node, before we made roles pluggable, has no possibility of understanding a remote_cluster_client role, it would blow up if it sees it

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

public static Set<DiscoveryNodeRole> BUILT_IN_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE);
public static Set<DiscoveryNodeRole> BUILT_IN_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE, REMOTE_CLUSTER_CLIENT_ROLE);

static Set<DiscoveryNodeRole> LEGACY_ROLES = Set.of(DATA_ROLE, INGEST_ROLE, MASTER_ROLE);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation

@jasontedor jasontedor merged commit 1fc0432 into elastic:master Mar 24, 2020
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 24, 2020
This commit introduce a formal role for identifying nodes that are
capable of making connections to remote clusters.
jasontedor added a commit that referenced this pull request Mar 25, 2020
This commit introduce a formal role for identifying nodes that are
capable of making connections to remote clusters.

Relates #53924
@jasontedor jasontedor deleted the remote-cluster-client-role branch March 25, 2020 01:59
yyff pushed a commit to yyff/elasticsearch that referenced this pull request Apr 17, 2020
This commit introduce a formal role for identifying nodes that are
capable of making connections to remote clusters.
dnhatn added a commit that referenced this pull request Jul 14, 2020
)

The primary shards of follower indices during the bootstrap need to be
on nodes with the remote cluster client role as those nodes reach out to
the corresponding leader shards on the remote cluster to copy Lucene
segment files and renew the retention leases. This commit introduces a
new allocation decider that ensures bootstrapping follower primaries are
allocated to nodes with the remote cluster client role.

Relates #54146
Relates #53924
Closes #58534

Co-authored-by: Jason Tedor <jason@tedor.me>
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 14, 2020
…stic#59375)

The primary shards of follower indices during the bootstrap need to be
on nodes with the remote cluster client role as those nodes reach out to
the corresponding leader shards on the remote cluster to copy Lucene
segment files and renew the retention leases. This commit introduces a
new allocation decider that ensures bootstrapping follower primaries are
allocated to nodes with the remote cluster client role.

Relates elastic#54146
Relates elastic#53924
Closes elastic#58534

Co-authored-by: Jason Tedor <jason@tedor.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >enhancement v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants