Register CcrRepository based on settings update by Tim-Brooks · Pull Request #36086 · elastic/elasticsearch

Tim-Brooks · 2018-11-29T22:09:07Z

This commit adds an empty CcrRepository snapshot/restore repository.
When a new cluster is registered in the remote cluster settings, a new
CcrRepository is registered for that cluster.

This is implemented using a new concept of "internal repositories".
RepositoryPlugin now allows implementations to return factories for
"internal repositories". The "internal repositories" are different from
normal repositories in that they cannot be registered through the
external repository api. Additionally, "internal repositories" are local
to a node and are not stored in the cluster state.

The repository will be unregistered if the remote cluster is removed.

…rnal

elasticmachine · 2018-11-29T22:09:09Z

Pinging @elastic/es-distributed

Tim-Brooks · 2018-11-30T00:23:13Z

@bleskes - In response to your comment here:

#35801 (comment)

Since we are using the client for the propagation to RepositoriesService here, an integration test seemed most straightforward. But I added a TODO to roll that integration test in more encompassing IT as the bootstrap work expands.

ywelsch

Thanks for the PR @tbrooks8. I've left some initial comments. I think we should also do more unit-level testing. Start e.g. with the newly added methods to RepositoriesService. They can easily be unit-tested. The repo manager might also allow some unit-tests.

ywelsch · 2018-11-30T17:02:51Z

...g/elasticsearch/action/admin/cluster/repositories/delete/DeleteInternalRepositoryAction.java

+import org.elasticsearch.action.support.master.AcknowledgedResponse;
+import org.elasticsearch.common.io.stream.Writeable;
+
+public class DeleteInternalRepositoryAction extends Action<AcknowledgedResponse> {


can you move these actions to the CCR plugin?

Sure. Although I will note that the conception on "internal repositories" still exist in open source. even if the actions to manipulate them do not.

Do you want me to change the names from what they are such as:

"cluster:admin/internal_repository/put"

to something ccr oriented?

yes, same for the action names and requests. For now, these can be DeleteInternalCCRRepositoryAction, DeleteInternalCCRRepositoryRequest, ...

ywelsch · 2018-11-30T17:07:49Z

...g/elasticsearch/action/admin/cluster/repositories/delete/DeleteInternalRepositoryAction.java

+    }
+
+    @Override
+    public AcknowledgedResponse newResponse() {


why AcknowledgedResponse? You're not interested in checking the acknowledged flag, so maybe just an ActionResponse.

ywelsch · 2018-11-30T17:09:24Z

.../elasticsearch/action/admin/cluster/repositories/delete/DeleteInternalRepositoryRequest.java

+
+public class DeleteInternalRepositoryRequest extends ActionRequest {
+
+    private String name;


ywelsch · 2018-11-30T17:13:04Z

.../elasticsearch/action/admin/cluster/repositories/delete/DeleteInternalRepositoryRequest.java

+    public ActionRequestValidationException validate() {
+        ActionRequestValidationException validationException = null;
+        if (name == null) {
+            validationException = addValidationError("name is missing", validationException);


maybe easier to check this right away on object creation, i.e., this.name = Objects.requireNonNull(name);

ywelsch · 2018-11-30T17:14:04Z

...va/org/elasticsearch/action/admin/cluster/repositories/put/PutInternalRepositoryRequest.java

+
+    private String name;
+    private String type;
+    private Settings settings;


ywelsch · 2018-11-30T17:46:13Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

+            closeRepository(repository);
+            repository.close();
+        } else {
+            logger.warn(() -> new ParameterizedMessage("Attempted to unregistered internal repository [{}][{}]. " +


ywelsch · 2018-11-30T17:47:22Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

+        Repository existingRepository = internalRepositories.putIfAbsent(name, repository);
+
+        if (existingRepository != null) {
+            logger.error(new ParameterizedMessage("Error registering internal repository [{}][{}]. " +


start logging messages with lower case (in style with the rest of the class / code)?

ywelsch · 2018-11-30T17:49:04Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java

    private final boolean enabled;
    private final Settings settings;
    private final CcrLicenseChecker ccrLicenseChecker;
+    private SetOnce<ClusterService> clusterService = new SetOnce<>();


this is not used anywhere?

ywelsch · 2018-11-30T17:49:11Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java

    private final Settings settings;
    private final CcrLicenseChecker ccrLicenseChecker;
+    private SetOnce<ClusterService> clusterService = new SetOnce<>();
+    private SetOnce<CcrRepositoryManager> repositoryManager = new SetOnce<>();


ywelsch · 2018-11-30T17:57:51Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrRepositoryManager.java

+class CcrRepositoryManager extends RemoteClusterAware {
+
+    private final NodeClient client;
+    private final Set<String> clusters = ConcurrentCollections.newConcurrentSet();


instead of caching this list here, can we directly ask RepositoriesService whether it already has a repo for this thing, and add / remove based on what RepositoriesService has?
Having these extra caches are always tricky, in particular when there are failure scenarios and the list of internal repositories in RepositoriesService might go out of sync with the cached clusters here.

Since put and delete should be idempotent (and they other for other normal repositories requests) I removed the cache and just call the action each time.

The other option we to create a get action, but I thought that was unnecessary as we need to handle potential concurrency in the put and delete methods anyway.

I removed the cache and just call the action each time.

sounds good

ywelsch · 2018-12-03T11:16:35Z

server/src/main/java/org/elasticsearch/action/ActionModule.java


-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;


ywelsch · 2018-12-03T11:31:47Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesModule.java

+            Map<String, Repository.Factory> newRepoTypes = repoPlugin.getInternalRepositories(env, namedXContentRegistry);
+            for (Map.Entry<String, Repository.Factory> entry : newRepoTypes.entrySet()) {
+                if (internalFactories.put(entry.getKey(), entry.getValue()) != null) {
+                    throw new IllegalArgumentException("Internal repository type [" + entry.getKey() + "] is already registered");


should we enforce that these types are distinct to the non-internal ones?

add a test for this?

ywelsch · 2018-12-03T12:08:02Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

+        Repository newRepository = createRepository(metaData, internalTypesRegistry);
+        Repository repositoryToClose = null;
+        boolean updated = false;
+        synchronized (internalRepositories) {


the concurrency logic looks complicated here. Maybe just add synchronized on the registerInternalRepository and unregisterInternalRepository methods? We will not be calling those concurrently anyway?

After the changes where we do not support updates, we can rely on the ConcurrentMap to provide concurrency control.

ywelsch · 2018-12-03T12:12:08Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

        Map<String, OriginalIndices> originalIndicesMap = new HashMap<>();
        if (isCrossClusterSearchEnabled()) {
-            final Map<String, List<String>> groupedIndices = groupClusterIndices(indices, indexExists);
+            final Map<String, List<String>> groupedIndices = groupClusterIndices(remoteClusters.keySet(), indices, indexExists);


use getRemoteClusterNames() here?

ywelsch · 2018-12-03T12:17:58Z

...ain/java/org/elasticsearch/xpack/ccr/action/repositories/DeleteInternalRepositoryAction.java

+
+    @Override
+    public Writeable.Reader<ActionResponse> getResponseReader() {
+        return in -> new ActionResponse() {};


safer to use the ActionResponse(StreamInput in) constructor here. Maybe use add a dummy response sub-class here.

ywelsch · 2018-12-03T12:20:37Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrRepositoryManager.java

+            client.executeLocally(DeleteInternalRepositoryAction.INSTANCE, request, future);
+            assert future.isDone() : "Should be completed as it is executed synchronously";
+        } else {
+            ActionRequest request = new PutInternalRepositoryRequest(clusterAlias, CcrRepository.TYPE);


instead of using the clusterAlias name verbatim here, let's prepend something like "ccr" to avoid name conflicts with standard repositories.

ywelsch · 2018-12-03T12:22:49Z

...g/elasticsearch/action/admin/cluster/repositories/delete/DeleteInternalRepositoryAction.java

+import org.elasticsearch.action.support.master.AcknowledgedResponse;
+import org.elasticsearch.common.io.stream.Writeable;
+
+public class DeleteInternalRepositoryAction extends Action<AcknowledgedResponse> {


yes, same for the action names and requests. For now, these can be DeleteInternalCCRRepositoryAction, DeleteInternalCCRRepositoryRequest, ...

ywelsch · 2018-12-03T12:23:50Z

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/PutInternalRepositoryRequest.java

+        this(name, type, Settings.EMPTY);
+    }
+
+    public PutInternalRepositoryRequest(String name, String type, Settings settings) {


If we rename this to PutInternalCCRRepositoryRequest, we can get rid of the settings parameter here. We don't use it for CCR.

ywelsch · 2018-12-03T12:28:56Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java

+
+        // TODO: Normally we would do validation when we update a repository to ensure that it is not in use.
+        //  Are we okay with not including that validation under the assumption that internal operations
+        //  will do the right thing.


I think it's fine not having this validation here. If we remove the settings parameter from this method, a repo will never be updated with different settings.

ywelsch · 2018-12-03T12:30:42Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrRepositoryManager.java

+class CcrRepositoryManager extends RemoteClusterAware {
+
+    private final NodeClient client;
+    private final Set<String> clusters = ConcurrentCollections.newConcurrentSet();


I removed the cache and just call the action each time.

sounds good

ywelsch

I've left a few more comments. Looking great already.

…rnal

ywelsch

2 smaller asks, looks good o.w.

ywelsch · 2018-12-04T12:59:46Z

server/src/main/java/org/elasticsearch/repositories/RepositoriesModule.java

+            Map<String, Repository.Factory> newRepoTypes = repoPlugin.getInternalRepositories(env, namedXContentRegistry);
+            for (Map.Entry<String, Repository.Factory> entry : newRepoTypes.entrySet()) {
+                if (internalFactories.put(entry.getKey(), entry.getValue()) != null) {
+                    throw new IllegalArgumentException("Internal repository type [" + entry.getKey() + "] is already registered");


add a test for this?

ywelsch · 2018-12-04T13:20:19Z

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrRepositoryManagerIT.java

+        assertAcked(followerClient().admin().cluster().updateSettings(putFollowerRequest).actionGet());
+
+        String followerCopyRepoName = CcrRepository.NAME_PREFIX + "follower_cluster_copy";
+        assertBusy(() -> {


the assertBusy should not be needed here. The update settings call will only return if the corresponding cluster state has been updated on all nodes, and the repositories are created as part of that CS update.

…rnal

Tim-Brooks · 2018-12-04T20:19:20Z

run the docbldesx

Tim-Brooks · 2018-12-04T20:22:56Z

run default distro tests

Tim-Brooks · 2018-12-04T21:25:37Z

run the docbldesx

This is a follow-up to elastic#36086. It renames the internal repository actions to be prefixed by "internal". This allows the system user to execute the actions. Additionally, this PR stops casting Client to NodeClient. The client we have is a NodeClient so executing the actions will be local.

This is a follow-up to #36086. It renames the internal repository actions to be prefixed by "internal". This allows the system user to execute the actions. Additionally, this PR stops casting Client to NodeClient. The client we have is a NodeClient so executing the actions will be local.

This commit adds an empty CcrRepository snapshot/restore repository. When a new cluster is registered in the remote cluster settings, a new CcrRepository is registered for that cluster. This is implemented using a new concept of "internal repositories". RepositoryPlugin now allows implementations to return factories for "internal repositories". The "internal repositories" are different from normal repositories in that they cannot be registered through the external repository api. Additionally, "internal repositories" are local to a node and are not stored in the cluster state. The repository will be unregistered if the remote cluster is removed.

This is a follow-up to elastic#36086. It renames the internal repository actions to be prefixed by "internal". This allows the system user to execute the actions. Additionally, this PR stops casting Client to NodeClient. The client we have is a NodeClient so executing the actions will be local.

This is a follow-up to #36086. It renames the internal repository actions to be prefixed by "internal". This allows the system user to execute the actions.

Tim-Brooks added 12 commits November 26, 2018 16:38

Work RepositoriesService into CCR

c92a1c7

WIP

a2c7576

WIP

e1b260e

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

b637022

…rnal

Work on creating actions

e45dfe7

WIP

68823cc

WIP

719e7e8

WIP

1949578

WIP

0f77fce

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

3852087

…rnal

WIP

3dd4a44

WIP

8bf1a1d

Tim-Brooks added >non-issue v7.0.0 :Distributed/CCR Issues around the Cross Cluster State Replication features v6.6.0 labels Nov 29, 2018

comment

9dbf0c3

Tim-Brooks mentioned this pull request Nov 29, 2018

Register CcrRepository based on settings update #35801

Closed

Fix checkstyle

9e4736e

Tim-Brooks requested review from bleskes, jasontedor, martijnvg and ywelsch November 30, 2018 00:20

ywelsch suggested changes Nov 30, 2018

View reviewed changes

Tim-Brooks added 2 commits November 30, 2018 14:36

Changes from review

8f11a77

Changes from review

4d58c11

Tim-Brooks requested a review from ywelsch November 30, 2018 23:06

Fix licenses

a15bffd

ywelsch reviewed Dec 3, 2018

View reviewed changes

ywelsch suggested changes Dec 3, 2018

View reviewed changes

Tim-Brooks added 4 commits December 3, 2018 12:34

Changes for review

6cb2750

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

777ece2

…rnal

Changes

0259d7d

Add validation

a7400cc

Tim-Brooks requested a review from ywelsch December 4, 2018 00:54

ywelsch approved these changes Dec 4, 2018

View reviewed changes

Tim-Brooks added 4 commits December 4, 2018 10:25

Changes for review

2dd4981

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

987c5a5

…rnal

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

d5b9de8

…rnal

Merge remote-tracking branch 'upstream/master' into add_ccr_repo_inte…

c73877a

…rnal

Tim-Brooks merged commit 8bde608 into elastic:master Dec 4, 2018

Tim-Brooks mentioned this pull request Dec 5, 2018

Rename internal repository actions to be internal #36244

Merged

Tim-Brooks added the backport pending label Dec 5, 2018

Tim-Brooks removed the backport pending label Dec 7, 2018

Tim-Brooks mentioned this pull request Dec 7, 2018

Rename internal repository actions to be internal #36377

Merged

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

Tim-Brooks deleted the add_ccr_repo_internal branch December 18, 2019 14:46


		public class DeleteInternalRepositoryRequest extends ActionRequest {

		private String name;

Conversation

Tim-Brooks commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 29, 2018

Uh oh!

Tim-Brooks commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Dec 4, 2018

Uh oh!

Tim-Brooks commented Dec 4, 2018

Tim-Brooks commented Nov 29, 2018 •

edited

Loading

Tim-Brooks commented Nov 30, 2018 •

edited

Loading

ywelsch left a comment •

edited

Loading