Refactor AutoFollowCoordinator to track leader indices per remote cluster #36031
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martijnvg
added
>enhancement
v7.0.0
:Distributed Indexing/CCR
|
Pinging @elastic/es-distributed |
martijnvg
commented
Nov 29, 2018
| ElasticsearchSecurityException.class, | ||
| "current license is non-compliant for [ccr]")); | ||
| Loggers.addAppender(logger, appender); | ||
| new MockLogAppender.ExceptionSeenEventExpectation( |
There was a problem hiding this comment.
Before the auto following was bootstrapped from thread pool scheduler,
but now auto followers for new remote clusters are bootstrapped when
a new cluster state is published.
…te cluster and replaced poll interval setting with a hardcoded poll interval. The hard coded interval will be removed in a follow up change to make use of cluster state API's wait_for_metatdata_version. Originates from elastic#35895 Relates to elastic#33007
to mock appender misses the expected log line. Before the auto following was bootstrapped from thread pool scheduler, but now auto followers for new remote clusters are bootstrapped when a new cluster state is published.
martijnvg
force-pushed
the
ccr_autofollower_per_cluster
branch
from
22d54cf to
5fe75b0
Compare
jasontedor
reviewed
Dec 4, 2018
| @@ -71,4 +77,53 @@ public void testFollowIndex() throws Exception { | |||
| } | |||
| } | |||
|
|
|||
| public void testAutoFollowPatterns() throws Exception { | |||
There was a problem hiding this comment.
The purpose of this test appears to be to test auto-following against two clusters. I am fine with reusing the infrastructure from the chain tests for this but I have a few comments:
- let us not use the
ChainITclass as that one is about testing chain replication - let us rename the sub-project from
chainsince this is no longer about testing chain replication only
There was a problem hiding this comment.
Coming up with a good name for the chain qa module is difficult, since many qa modules names have multi-cluster in their name.
What about removing the multi-cluster prefix from multi-cluster-downgraded-to-basic-license, multi-cluster-with-incompatible-license, multi-cluster-with-non-compliant-license and multi-cluster-with-security qa modules name. At the same time then we can merge multi-cluster and chain qa modules. The new qa module with the name multi-cluster will start 3 clusters, so tests from both modules should work. If you think this makes sense then I think we should do this in a followup PR.
| Set<String> newRemoteClusters = autoFollowMetadata.getPatterns().entrySet().stream() | ||
| .filter(entry -> autoFollowers.containsKey(entry.getValue().getRemoteCluster()) == false) | ||
| .map(entry -> entry.getValue().getRemoteCluster()) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
Doing the map to the remote cluster first makes the code a little clearer and simpler:
diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java
index f4d4cfa23c4..586eea70510 100644
--- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java
+++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java
@@ -140,9 +140,9 @@ public class AutoFollowCoordinator implements ClusterStateListener {
final CopyOnWriteHashMap<String, AutoFollower> autoFollowers = CopyOnWriteHashMap.copyOf(this.autoFollowers);
Set<String> newRemoteClusters = autoFollowMetadata.getPatterns().entrySet().stream()
- .filter(entry -> autoFollowers.containsKey(entry.getValue().getRemoteCluster()) == false)
- .map(entry -> entry.getValue().getRemoteCluster())
- .collect(Collectors.toSet());
+ .map(entry -> entry.getValue().getRemoteCluster())
+ .filter(remoteCluster -> autoFollowers.containsKey(remoteCluster) == false)
+ .collect(Collectors.toSet());
Map<String, AutoFollower> newAutoFollowers = new HashMap<>(newRemoteClusters.size());
for (String remoteCluster : newRemoteClusters) {| private volatile AtomicArray<AutoFollowResult> autoFollowResults; | ||
|
|
||
| AutoFollower(final String remoteCluster, | ||
| ThreadPool threadPool, final Consumer<List<AutoFollowResult>> statsUpdater, |
There was a problem hiding this comment.
Nit: there are two parameters on this line
martijnvg
added a commit
that referenced
this pull request
Dec 5, 2018
…ster (#36031) and replaced poll interval setting with a hardcoded poll interval. The hard coded interval will be removed in a follow up change to make use of cluster state API's wait_for_metatdata_version. Before the auto following was bootstrapped from thread pool scheduler, but now auto followers for new remote clusters are bootstrapped when a new cluster state is published. Originates from #35895 Relates to #33007
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Dec 8, 2018
Renamed the follow qa modules: `multi-cluster-downgraded-to-basic-license` to `downgraded-to-basic-license` `multi-cluster-with-non-compliant-license` to `non-compliant-license` `multi-cluster-with-security` to `security` Moved the `chain` module into the `multi-cluster` module and changed the `multi-cluster` to start 3 clusters. Followup from elastic#36031
martijnvg
added a commit
that referenced
this pull request
Dec 9, 2018
Renamed the follow qa modules: `multi-cluster-downgraded-to-basic-license` to `downgraded-to-basic-license` `multi-cluster-with-non-compliant-license` to `non-compliant-license` `multi-cluster-with-security` to `security` Moved the `chain` module into the `multi-cluster` module and changed the `multi-cluster` to start 3 clusters. Followup from #36031
martijnvg
added a commit
that referenced
this pull request
Dec 9, 2018
Renamed the follow qa modules: `multi-cluster-downgraded-to-basic-license` to `downgraded-to-basic-license` `multi-cluster-with-non-compliant-license` to `non-compliant-license` `multi-cluster-with-security` to `security` Moved the `chain` module into the `multi-cluster` module and changed the `multi-cluster` to start 3 clusters. Followup from #36031
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
and replaced poll interval setting with a hardcoded poll interval.
The hard coded interval will be removed in a follow up change to make
use of cluster state API's wait_for_metatdata_version.
Originates from #35895
Relates to #33007