Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register CcrRepository based on settings update #35801

Closed
wants to merge 9 commits into from

Conversation

Tim-Brooks
Copy link
Contributor

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 based on the
master nodes view of the settings. So if a new node becomes master, it
refreshes the repositories based on the remote clusters it is aware of.

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

@Tim-Brooks Tim-Brooks added >non-issue v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.6.0 labels Nov 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tbrooks8 . I left some comments

if (repositories == null) {
List<RepositoryMetaData> repositoriesMetaData = new ArrayList<>(clusters.size());
for (String cluster : clusters) {
LOGGER.info("put [{}] repository [{}]", CcrRepository.TYPE, cluster);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be debug.


if (repositories == null) {
List<RepositoryMetaData> repositoriesMetaData = new ArrayList<>(clusters.size());
for (String cluster : clusters) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of relying on a cached version, maybe parse the cluster state and get the current list of remote clusters? we can maybe add static methods in the RemoteClusterAware class to parse the settings into a whatever we need


@Override
public void onMaster() {
this.isMasterNode = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of caching this - why not ask the cluster service if the current node is master every time we want to know? we already have a reference to it.


@Override
protected void updateRemoteCluster(String clusterAlias, List<String> addresses, String proxyAddress) {
if (addresses.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this method be called multiple times if multiple clusters changed?


import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;

public class CcrRepositoryManagerIT extends CcrIntegTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to have an IntegTest for this? I presume we will future tests that will cover this and more? can't we have a unit tests that checks how the CCRRepositoryManager reacts to cluster state changes?

@Tim-Brooks
Copy link
Contributor Author

Closed in favor of approach in #36086.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >non-issue v6.6.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants