-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Use CcrRepository
to init follower index (#35719)
#37988
Use CcrRepository
to init follower index (#35719)
#37988
Conversation
This commit modifies the put follow index action to use a CcrRepository when creating a follower index. It routes the logic through the snapshot/restore process. A wait_for_active_shards parameter can be used to configure how long to wait before returning the response.
Pinging @elastic/es-distributed |
@elasticmachine run elasticsearch-ci/2 |
1 similar comment
@elasticmachine run elasticsearch-ci/2 |
@ywelsch I added a pre-6.7 put follow mode. The code is copied from the prior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only reviewed eea717e (the extra changes for 6.7) and left 2 comments.
...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutFollowAction.java
Outdated
Show resolved
Hide resolved
@@ -134,7 +135,7 @@ public void checkRemoteClusterLicenseAndFetchLeaderIndexMetadataAndHistoryUUIDs( | |||
hasPrivilegesToFollowIndices(remoteClient, new String[] {leaderIndex}, e -> { | |||
if (e == null) { | |||
fetchLeaderHistoryUUIDs(remoteClient, leaderIndexMetaData, onFailure, historyUUIDs -> | |||
consumer.accept(historyUUIDs, leaderIndexMetaData)); | |||
consumer.accept(historyUUIDs, new Tuple<>(remoteClusterState, leaderIndexMetaData))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to set .nodes(true)
on the cluster state request in order to get the minNodeVersion? Why is this not failing any tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodes
defaults to true. But I will set it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well I see that we set clear()
which should reset it. I'll investigate why the test did not fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that the test was passing (I believe based on reproductions locally) is that the rolling upgrade uses the local cluster as the remote cluster. So the check that your local cluster was mixed with pre-6.7 was pushing us to use compatibility mode. I changed the remote cluster state request to request nodes(true)
.
This does raise questions about testing, do we need to invest in rolling upgrade tests that use two different cluster and have a test where your local cluster is all 6.7 and the remote cluster is mixed?
@elasticmachine run elasticsearch-ci/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
final class Pre67PutFollow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 too easy to forget about bwc.
Did you notice the ccr tests in rolling upgrade tests fail when this was missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Not always but sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit modifies the put follow index action to use a
CcrRepository when creating a follower index. It routes
the logic through the snapshot/restore process. A
wait_for_active_shards parameter can be used to configure
how long to wait before returning the response.