[CCR] Added more validation to follow index api. #31068
Conversation
martijnvg
added
review
:Distributed Indexing/CCR
|
Pinging @elastic/es-distributed |
| } | ||
| Map<String, Object> leaderMapping = getMapping(leaderIndex); | ||
| Map<String, Object> followerMapping = getMapping(followIndex); | ||
| if (leaderMapping.equals(followerMapping) == false) { |
There was a problem hiding this comment.
FYI - for future work - they should be mergeable (i.e. leader into follower) not necessarily identical.
There was a problem hiding this comment.
That makes sense. The shard follow tasks also add the missing fields in the case they are missing at startup.
There was a problem hiding this comment.
done here: 284e2ddd6f2c6da0d0255d6a8d45d8cd6e9ebddf
| throw new IllegalArgumentException("the leader and follower mappings must be identical"); | ||
| } | ||
| Map<String, Settings> leaderAnalysisSettings = leaderIndex.getSettings().getGroups("index.analysis"); | ||
| Map<String, Settings> followerAnalysisSettings = followIndex.getSettings().getGroups("index.analysis"); |
There was a problem hiding this comment.
I wonder if we should flip this and only while list things that can be different (like number of replicas, allocation filtering etc.). Thoughts?
There was a problem hiding this comment.
This does seem safer, however almost all settings are allowed to be different so the whitelist is going to be very large then.
There was a problem hiding this comment.
This is indeed what I wasn't sure about. I did a very brief random sample of IndexSetting and I'm not sure we need to white list so many settings when compared to all the settings that are in there?
There was a problem hiding this comment.
So you mean that we only whitelist a few settings (like refresh_rate) while we could whitelist more settings (like translog settings) but since there are not commonly used we just not allow it?
There was a problem hiding this comment.
to be clear - by not whitelisting them it doesn't mean people can't change them - it means they need to be same at both source and target. I think things like refresh rate and and translog settings are ok in that regard?
There was a problem hiding this comment.
to be clear - by not whitelisting them it doesn't mean people can't change them - it means they need to be same at both source and target.
Yes.
I think things like refresh rate and and translog settings are ok in that regard?
What settings do you think are likely to be different in leader and follower index?
There was a problem hiding this comment.
@bleskes I've added a whitelist of allowed settings that may be different: 38f2cf188cfd119bd178d8d1bceff5957f30dec0
martijnvg
force-pushed
the
ccr_more_more_validation
branch
from
ac47a9b to
284e2dd
Compare
martijnvg
force-pushed
the
ccr_more_more_validation
branch
from
38f2cf1 to
9e57d12
Compare
| try { | ||
| start(request, null, leaderIndexMetadata, followIndexMetadata, listener); | ||
| } catch (IOException e) { | ||
| listener.onFailure(e); |
There was a problem hiding this comment.
add a return method here just in case? I don't like this construct but can't think of how to improve it (all other options I see suck too)
There was a problem hiding this comment.
I don't think it is really needed, because this were the if code block ends and below it is an else code block and that is it.
There was a problem hiding this comment.
I know it's not needed now but it's a bug waiting to happen when someone adds a line below.
| validate (leaderIndexMetadata ,followIndexMetadata , request); | ||
| ActionListener<Response> handler) throws IOException { | ||
| MapperService mapperService = followIndexMetadata != null ? indicesService.createIndexMapperService(followIndexMetadata) : null; | ||
| validate(request, leaderIndexMetadata, followIndexMetadata, mapperService); |
There was a problem hiding this comment.
can you fix the indentation here?
| assertThat(e.getMessage(), | ||
| equalTo("leader index primary shards [5] does not match with the number of shards of the follow index [4]")); | ||
| } | ||
| { |
There was a problem hiding this comment.
can you add a comment to each of these that indicates what you're testing? that way you don't need to squint at the setting and figure out what's different.
martijnvg
force-pushed
the
ccr_more_more_validation
branch
from
9e57d12 to
8f6ac19
Compare
|
retest this please |
1 similar comment
|
retest this please |
… leader and follow index
martijnvg
force-pushed
the
ccr_more_more_validation
branch
from
8f6ac19 to
892e175
Compare
Relates to #30086