Decouple ClusterStateTaskListener & ClusterApplier by DaveCTurner · Pull Request #30809 · elastic/elasticsearch

DaveCTurner · 2018-05-23T10:37:53Z

Today, the ClusterApplier and MasterService both use the
ClusterStateTaskListener interface to notify their callers when asynchronous
activities have completed. However, this is not wholly appropriate: none of the
callers into the ClusterApplier care about the ClusterState arguments that
they receive. This change introduces a dedicated ClusterApplyListener
interface for callers into the ClusterApplier, to distinguish these listeners
from the real ClusterStateTaskListeners that are waiting for responses from
the MasterService.

Today, the `ClusterApplier` and `MasterService` both use the `ClusterStateTaskListener` interface to notify their callers when asynchronous activities have completed. However, this is not wholly appropriate: none of the callers into the `ClusterApplier` care about the `ClusterState` arguments that they receive. This change introduces a dedicated ClusterApplyListener interface for callers into the `ClusterApplier`, to distinguish these listeners from the real `ClusterStateTaskListener`s that are waiting for responses from the `MasterService`.

elasticmachine · 2018-05-23T10:37:54Z

Pinging @elastic/es-distributed

DaveCTurner · 2018-05-23T10:39:45Z

server/src/main/java/org/elasticsearch/cluster/service/ClusterApplierService.java

                logger.error(new ParameterizedMessage(
-                        "exception thrown by listener while notifying of cluster state processed from [{}], old cluster state:\n" +
-                            "{}\nnew cluster state:\n{}",
-                        source, oldState, newState), e);


The only place where oldState and newState are used is in this log message.

DaveCTurner · 2018-05-23T10:40:40Z

server/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java

        // TODO: give discovery module control over GatewayService
        clusterState = ClusterState.builder(clusterState).build();
-        clusterApplier.onNewClusterState("single-node-start-initial-join", () -> clusterState, (source, e) -> {});
+        clusterApplier.onNewClusterState("single-node-start-initial-join", () -> clusterState, new ClusterApplyListener() {


Possible improvement here: have a default (empty) onSuccess implementation so this can be a lambda.

DaveCTurner · 2018-05-23T10:42:00Z

server/src/main/java/org/elasticsearch/indices/store/IndicesStore.java

+                new ClusterApplyListener() {
+                    @Override
+                    public void onSuccess(String source) {
+                        logger.trace("{} successfully deleted unallocated shard", shardId);


Added this log message.

ywelsch

+1 on having a default implementation for onSuccess so that it can be used as lambda

ywelsch · 2018-05-23T12:47:38Z

server/src/main/java/org/elasticsearch/discovery/single/SingleNodeDiscovery.java

 package org.elasticsearch.discovery.single;

 import org.apache.logging.log4j.message.ParameterizedMessage;
+import org.elasticsearch.action.ActionListener;


I don't see this being used

DaveCTurner · 2018-05-23T13:30:32Z

Ok, I've added the default empty implementation, tidied up imports, and generally put things back to how they were a bit. Ready for another look.

ywelsch

LGTM

Today, the `ClusterApplier` and `MasterService` both use the `ClusterStateTaskListener` interface to notify their callers when asynchronous activities have completed. However, this is not wholly appropriate: none of the callers into the `ClusterApplier` care about the `ClusterState` arguments that they receive. This change introduces a dedicated ClusterApplyListener interface for callers into the `ClusterApplier`, to distinguish these listeners from the real `ClusterStateTaskListener`s that are waiting for responses from the `MasterService`.

* master: Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (elastic#30698) Limit user to single concurrent auth per realm (elastic#30794) [Tests] Move templated _rank_eval tests (elastic#30679) Security: fix dynamic mapping updates with aliases (elastic#30787) Ensure that ip_range aggregations always return bucket keys. (elastic#30701) Use remote client in TransportFieldCapsAction (elastic#30838) Move Watcher versioning setting to meta field (elastic#30832) [Docs] Explain incomplete dates in range queries (elastic#30689) Move persistent task registrations to core (elastic#30755) Decouple ClusterStateTaskListener & ClusterApplier (elastic#30809) Send client headers from TransportClient (elastic#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (elastic#30732) Force stable file modes for built packages (elastic#30823) [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport

* es/master: Move score script context from SearchScript to its own class (#30816) Fix bad version check writing Repository nodes (#30846) [docs] explainer for java packaging tests (#30825) Remove Throwable usage from transport modules (#30845) REST high-level client: add put ingest pipeline API (#30793) Update the version checks around ip_range bucket keys, now that the change was backported. Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Use geohash cell instead of just a corner in geo_bounding_box (#30698) Limit user to single concurrent auth per realm (#30794) [Tests] Move templated _rank_eval tests (#30679) Security: fix dynamic mapping updates with aliases (#30787) Ensure that ip_range aggregations always return bucket keys. (#30701) Use remote client in TransportFieldCapsAction (#30838) Move Watcher versioning setting to meta field (#30832) [Docs] Explain incomplete dates in range queries (#30689) Move persistent task registrations to core (#30755) Decouple ClusterStateTaskListener & ClusterApplier (#30809) Send client headers from TransportClient (#30803) Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732) Force stable file modes for built packages (#30823)

* es/6.x: Move score script context from SearchScript to its own class (#30816) Fix bad version check writing Repository nodes (#30846) Modify state of VerifyRepositoryResponse for bwc (#30762) QA: Fix tribe tests when running default zip Use remote client in TransportFieldCapsAction (#30838) Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Ensure that ip_range aggregations always return bucket keys. (#30701) Limit user to single concurrent auth per realm (#30794) Security: fix dynamic mapping updates with aliases (#30787) [Tests] Move templated _rank_eval tests (#30679) Move Watcher versioning setting to meta field (#30832) Restore "Add more yaml tests for get alias API " (#30814) Send client headers from TransportClient (#30803) [Docs] Explain incomplete dates in range queries (#30689) Move persistent task registrations to core (#30755) Decouple ClusterStateTaskListener & ClusterApplier (#30809) Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732) Force stable file modes for built packages (#30823)

DaveCTurner added >non-issue v7.0.0 >refactoring :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.4.0 labels May 23, 2018

DaveCTurner requested a review from ywelsch May 23, 2018 10:37

DaveCTurner commented May 23, 2018

View reviewed changes

ywelsch approved these changes May 23, 2018

View reviewed changes

DaveCTurner added 4 commits May 23, 2018 14:18

Introduce default empty onSuccess method

ca622b4

Whitespace and imports

bfbb629

Whitespace

f95f05c

Whitespace

64fe7a7

ywelsch approved these changes May 23, 2018

View reviewed changes

DaveCTurner added 2 commits May 23, 2018 16:21

Merge branch 'master' into 2018-05-23-cluster-applier-listener

ee0d094

Merge branch 'master' into 2018-05-23-cluster-applier-listener

efb6164

DaveCTurner merged commit ff0b6c7 into elastic:master May 24, 2018

DaveCTurner deleted the 2018-05-23-cluster-applier-listener branch May 24, 2018 08:05

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple ClusterStateTaskListener & ClusterApplier#30809

Decouple ClusterStateTaskListener & ClusterApplier#30809
DaveCTurner merged 7 commits intoelastic:masterfrom
DaveCTurner:2018-05-23-cluster-applier-listener

DaveCTurner commented May 23, 2018

Uh oh!

elasticmachine commented May 23, 2018

Uh oh!

DaveCTurner May 23, 2018

Uh oh!

DaveCTurner May 23, 2018

Uh oh!

DaveCTurner May 23, 2018

Uh oh!

ywelsch left a comment

Uh oh!

ywelsch May 23, 2018

Uh oh!

DaveCTurner commented May 23, 2018

Uh oh!

ywelsch left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

DaveCTurner commented May 23, 2018

Uh oh!

elasticmachine commented May 23, 2018

Uh oh!

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

ywelsch May 23, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented May 23, 2018

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants