Skip to content

Move data tier roles to server #71084

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Mar 31, 2021
Merged

Move data tier roles to server #71084

merged 23 commits into from
Mar 31, 2021

Conversation

@jasontedor
Copy link
Member

@jasontedor jasontedor commented Mar 30, 2021 •
edited
Loading

This commit moves the data tier roles to server. It is no longer necessary to separate these roles from server as we no longer build distributions that would not contain these roles. Moving these roles will simplify many things. This is deliberately the smallest possible commit that moves these roles. Other aspects related to the data tiers can move in separate, also small, commits.

Relates #71013

@jasontedor
Move data tier roles to server
e4a9746 
This commit moves the data tier roles to server. It is no longer
necessary to separate these roles from server as we no longer build
distributions that would not contain these roles. Moving these roles
will simplify many things. This is deliberately the smallest possible
commit that moves these roles. Other aspects related to the data tiers
can move in separate, also small, commits.
@elasticmachine elasticmachine added Team:Data Management Meta label for data/management team Team:Core/Infra Meta label for core/infra team labels Mar 30, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 30, 2021

Pinging @elastic/es-core-features (Team:Core/Features)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 30, 2021

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

dakrone
dakrone approved these changes Mar 30, 2021
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI is happy

@jasontedor jasontedor mentioned this pull request Mar 31, 2021
Merged
@jasontedor
Copy link
Member Author

jasontedor commented Mar 31, 2021

@dakrone I pushed a production code change to 4a73374 (for simplification, and correctness), can you please review. There was a failing test in DataTierTests#testNodeSelection that failed prior to 4a73374 and now passes, and I believe this change is future proof to us adding new roles.

@jasontedor jasontedor requested a review from dakrone March 31, 2021 01:51
@dakrone
Copy link
Member

dakrone commented Mar 31, 2021 •
edited
Loading

This reproduces a failure for me:

./gradlew :server:test --tests "org.elasticsearch.cluster.node.DiscoveryNodesTests.testCoordinatorOnlyNodes" -Dtests.seed=ABA5488DA8932ACB -Dtests.locale=no -Dtests.timezone=Atlantic/Jan_Mayen -Druntime.java=15

Suite: Test class org.elasticsearch.cluster.node.DiscoveryNodesTests
  1> [2021-03-31T05:21:19,218][INFO ][o.e.c.n.DiscoveryNodesTests] [testCoordinatorOnlyNodes] before test
  1> [2021-03-31T05:21:19,349][INFO ][o.e.c.n.DiscoveryNodesTests] [testCoordinatorOnlyNodes] after test
  2> REPRODUCE WITH: ./gradlew ':server:test' --tests "org.elasticsearch.cluster.node.DiscoveryNodesTests.testCoordinatorOnlyNodes" -Dtests.seed=ABA5488DA8932ACB -Dtests.security.manager=true -Dtests.locale=no -Dtests.timezone=Atlantic/Jan_Mayen -Druntime.java=15
  2> java.lang.AssertionError: 
    Expected: ["node_2"] in any order
         but: no item matches: "node_2" in []
        at __randomizedtesting.SeedInfo.seed([ABA5488DA8932ACB:FB49285E5818C88]:0)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
        at org.junit.Assert.assertThat(Assert.java:956)
        at org.junit.Assert.assertThat(Assert.java:923)
        at org.elasticsearch.cluster.node.DiscoveryNodesTests.testCoordinatorOnlyNodes(DiscoveryNodesTests.java:113)

henningandersen
henningandersen approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this.

server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNodes.java
resolvedNodesIds.removeAll(ingestNodes.keys());
mutation = resolvedNodesIds::remove;
}
} else if (DiscoveryNode.COORDINATING_ONLY.equals(matchAttrName)) {
Copy link
Contributor

@henningandersen henningandersen Mar 31, 2021

Choose a reason for hiding this comment

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

I believe this case is no longer handled. COORDINATING_ONLY is not included in BUILTIN_ROLES, but is allowed in node specifications.

DiscoveryNodesTests.testCoordinatorOnlyNodes fails like 1/3 of the time.

@jasontedor jasontedor merged commit e119ac6 into elastic:master Mar 31, 2021
@mark-vieira mark-vieira mentioned this pull request Mar 31, 2021
Closed
@jasontedor jasontedor mentioned this pull request Mar 31, 2021
Merged
@jasontedor jasontedor deleted the move-data-roles branch March 31, 2021 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningandersen henningandersen henningandersen approved these changes

@dakrone dakrone Awaiting requested review from dakrone

Assignees

No one assigned

Labels

:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team v7.13.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jasontedor @elasticmachine @dakrone @henningandersen @jakelandis