Decouple ClusterStateTaskListener & ClusterApplier #30809

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

            } catch (Exception e) {
                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

@@ -101,7 +103,15 @@ public synchronized void startInitialJoin() {
        // apply a fresh cluster state just so that state recovery gets triggered by GatewayService
        // 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

@@ -20,13 +20,15 @@
 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 Coordination/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 commented May 23, 2018

elasticmachine commented May 23, 2018

DaveCTurner May 23, 2018

DaveCTurner May 23, 2018

DaveCTurner May 23, 2018

ywelsch left a comment

ywelsch May 23, 2018

DaveCTurner commented May 23, 2018

ywelsch left a comment

Decouple ClusterStateTaskListener & ClusterApplier #30809

Decouple ClusterStateTaskListener & ClusterApplier #30809

Conversation

DaveCTurner commented May 23, 2018

elasticmachine commented May 23, 2018

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

DaveCTurner May 23, 2018

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch May 23, 2018

Choose a reason for hiding this comment

DaveCTurner commented May 23, 2018

ywelsch left a comment

Choose a reason for hiding this comment