SNAPSHOT: Repo Creation out of ClusterStateTask #36157
Conversation
* Move `createRepository` call out of cluster state tasks
* Now only `RepositoriesService#applyClusterState` manipulates `this.repositories`
* Closes elastic#9488
original-brownbear
added
>bug
WIP
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed |
| Repository newRepo = createRepository(repositoryMetaData); | ||
| createRepository(repositoryMetaData); | ||
| if (previous != null) { | ||
| closeRepository(previous); |
There was a problem hiding this comment.
that looks dangerous now (i.e., closing the existing repo). You should rather close the temporarily created repository.
There was a problem hiding this comment.
Done, the existing one is closed in org.elasticsearch.repositories.RepositoriesService#applyClusterState anyway as far as I can see :)
| */ | ||
| private boolean registerRepository(RepositoryMetaData repositoryMetaData) throws IOException { | ||
| private boolean registerRepository(RepositoryMetaData repositoryMetaData) { | ||
| Repository previous = repositories.get(repositoryMetaData.name()); |
There was a problem hiding this comment.
I wonder if this logic is still needed. If we want to keep the logic to check if there have been no changes, we should just take the current repository metadata from the cluster state, and do this on the cluster state update task.
There was a problem hiding this comment.
you can then also inline this method so that it only becomes something like
try {
closeRepository(createRepository(newRepositoryMetaData));
} catch (Exception e) {
registrationListener.onFailure(e);
return;
}
There was a problem hiding this comment.
Makes sense :) Inlined as you suggest above and added a breakout to the cluster state task for the case of equal metadata.
|
@ywelsch thanks for taking a look! Applied your suggestions now :) |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
Can you add tests as well for the case where the repo with same settings exist? I'm not sure we have an existing test case for that (maybe look in RepositoriesIT)
|
@ywelsch thanks :) |
* Move `createRepository` call out of cluster state tasks
* Now only `RepositoriesService#applyClusterState` manipulates `this.repositories`
* Closes #9488
4 participants
Addresses this comment #9488 (comment)
createRepositorycall out of cluster state tasksRepositoriesService#applyClusterStatemanipulatesthis.repositories