CCR: Do not follow if leader does not have soft-deletes #34767
Conversation
We should not create a follower index and abort a follow request if the leader does not have soft-deletes. Moreover, we also should not auto-follow an index if it does not have soft-deletes.
dnhatn
added
>non-issue
:Distributed Indexing/CCR
|
Pinging @elastic/es-distributed |
| @@ -370,7 +370,9 @@ private void finalise(int slot, AutoFollowResult result) { | |||
| // has a leader index uuid custom metadata entry that matches with uuid of leaderIndexMetaData variable | |||
| // If so then handle it differently: not follow it, but just add an entry to | |||
| // AutoFollowMetadata#followedLeaderIndexUUIDs | |||
| leaderIndicesToFollow.add(leaderIndexMetaData.getIndex()); | |||
| if (TransportPutFollowAction.canFollow(clusterAlias, leaderIndexMetaData) == null) { | |||
There was a problem hiding this comment.
Initially I wanted to do this check in checkAutoFollowPattern(...) method, so that we could fail or log when an index can't be followed, but then I realized that we would do this for every auto follow run and that would be way to noisy. So I think this is the right place.
| * Pre-flight check if the given index can be a leader index. | ||
| * This method returns {@code null} if no issue found; otherwise it returns the first issue that it found. | ||
| */ | ||
| static Exception canFollow(String leaderCluster, IndexMetaData leaderIndexMetaData) { |
There was a problem hiding this comment.
I don't think this needs to be a method shared between AutoFollowCoordinator and TransportPutFollowAction; I think that we can inline the check directly into the call sites. Then we can avoid having a debate about the method not throwing versus returning null versus returning an optional. 😇
There was a problem hiding this comment.
If we think there is value in having this method (for example, maybe for future purposes), then let us discuss that.
There was a problem hiding this comment.
The reason I introduced this method is for future purposes when we need to add one more check for the leader index. I am fine to inline it.
| @@ -125,6 +125,10 @@ private void createFollowerIndex( | |||
| listener.onFailure(new IllegalArgumentException("leader index [" + request.getLeaderIndex() + "] does not exist")); | |||
| return; | |||
| } | |||
| if (leaderIndexMetaData.getSettings().getAsBoolean(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), false) == false) { | |||
| listener.onFailure( | |||
| new IllegalArgumentException("leader index [" + request.getLeaderIndex() + "] does not enable soft-deletes")); | |||
There was a problem hiding this comment.
I think “does not have soft deletes enabled” was great!
|
Thanks @martijnvg and @jasontedor. |
We should not create a follower index and abort a follow request if the leader does not have soft-deletes. Moreover, we also should not auto-follow an index if it does not have soft-deletes.
We should not create a follower index and abort a follow request if the leader does not have soft-deletes. Moreover, we also should not auto-follow an index if it does not have soft-deletes.
We should not create a follower index and abort a follow request if the
leader does not have soft-deletes. Moreover, we also should not
auto-follow an index if it does not have soft-deletes.