[Bug] [Segment Replication] Update store metadata recovery diff logic to ignore missing files causing exception #4185
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,8 +110,8 @@ | |
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.locks.ReentrantReadWriteLock; | ||
|
|
@@ -1102,6 +1102,30 @@ public Map<String, StoreFileMetadata> asMap() { | |
| private static final String LIV_FILE_EXTENSION = "liv"; // lucene 5 delete file | ||
| private static final String SEGMENT_INFO_EXTENSION = "si"; | ||
|
|
||
| /** | ||
| * Helper method used to group store files according to segment and commit. | ||
| * | ||
| * @see MetadataSnapshot#recoveryDiff(MetadataSnapshot) | ||
| * @see MetadataSnapshot#segmentReplicationDiff(MetadataSnapshot) | ||
| */ | ||
| private Iterable<List<StoreFileMetadata>> getGroupedFilesIterable() { | ||
| final Map<String, List<StoreFileMetadata>> perSegment = new HashMap<>(); | ||
| final List<StoreFileMetadata> perCommitStoreFiles = new ArrayList<>(); | ||
| for (StoreFileMetadata meta : this) { | ||
| final String segmentId = IndexFileNames.parseSegmentName(meta.name()); | ||
| final String extension = IndexFileNames.getExtension(meta.name()); | ||
| if (IndexFileNames.SEGMENTS.equals(segmentId) | ||
| || DEL_FILE_EXTENSION.equals(extension) | ||
| || LIV_FILE_EXTENSION.equals(extension)) { | ||
| // only treat del files as per-commit files fnm files are generational but only for upgradable DV | ||
| perCommitStoreFiles.add(meta); | ||
| } else { | ||
| perSegment.computeIfAbsent(segmentId, k -> new ArrayList<>()).add(meta); | ||
| } | ||
| } | ||
| return Iterables.concat(perSegment.values(), Collections.singleton(perCommitStoreFiles)); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a diff between the two snapshots that can be used for recovery. The given snapshot is treated as the | ||
| * recovery target and this snapshot as the source. The returned diff will hold a list of files that are: | ||
|
|
@@ -1139,23 +1163,8 @@ public RecoveryDiff recoveryDiff(MetadataSnapshot recoveryTargetSnapshot) { | |
| final List<StoreFileMetadata> identical = new ArrayList<>(); | ||
| final List<StoreFileMetadata> different = new ArrayList<>(); | ||
| final List<StoreFileMetadata> missing = new ArrayList<>(); | ||
| final ArrayList<StoreFileMetadata> identicalFiles = new ArrayList<>(); | ||
| for (List<StoreFileMetadata> segmentFiles : getGroupedFilesIterable()) { | ||
| identicalFiles.clear(); | ||
| boolean consistent = true; | ||
| for (StoreFileMetadata meta : segmentFiles) { | ||
|
|
@@ -1190,6 +1199,51 @@ public RecoveryDiff recoveryDiff(MetadataSnapshot recoveryTargetSnapshot) { | |
| return recoveryDiff; | ||
| } | ||
|
|
||
| /** | ||
| * Segment Replication method | ||
| * Returns a diff between the two snapshots that can be used for getting list of files to copy over to a replica for segment replication. The given snapshot is treated as the | ||
| * target and this snapshot as the source. The returned diff will hold a list of files that are: | ||
| * <ul> | ||
| * <li>identical: they exist in both snapshots and they can be considered the same ie. they don't need to be recovered</li> | ||
| * <li>different: they exist in both snapshots but their they are not identical</li> | ||
| * <li>missing: files that exist in the source but not in the target</li> | ||
| * </ul> | ||
| */ | ||
| public RecoveryDiff segmentReplicationDiff(MetadataSnapshot recoveryTargetSnapshot) { | ||
|
There was a problem hiding this comment. Nit - rename snapshot as well for consistency - segmentReplicationTargetSnapshot There was a problem hiding this comment. Thanks @Poojita-Raj for the review. This method evaluates the difference in segment files b/w different store metadata snapshots which makes this naming consistent. This name also aligns with existing There was a problem hiding this comment. The code in this method seems identical to That would also remove the need to extract the There was a problem hiding this comment. Thanks @kartg, this is a good point! Initially, I thought of adding a flag for gating but later realized |
||
| final List<StoreFileMetadata> identical = new ArrayList<>(); | ||
| final List<StoreFileMetadata> different = new ArrayList<>(); | ||
| final List<StoreFileMetadata> missing = new ArrayList<>(); | ||
| final ArrayList<StoreFileMetadata> identicalFiles = new ArrayList<>(); | ||
| for (List<StoreFileMetadata> segmentFiles : getGroupedFilesIterable()) { | ||
| identicalFiles.clear(); | ||
| boolean consistent = true; | ||
| for (StoreFileMetadata meta : segmentFiles) { | ||
| StoreFileMetadata storeFileMetadata = recoveryTargetSnapshot.get(meta.name()); | ||
| if (storeFileMetadata == null) { | ||
| // Do not consider missing files as inconsistent in SegRep as replicas may lag while primary updates | ||
| // documents and generate new files specific to a segment | ||
| missing.add(meta); | ||
| } else if (storeFileMetadata.isSame(meta) == false) { | ||
| consistent = false; | ||
| different.add(meta); | ||
| } else { | ||
| identicalFiles.add(meta); | ||
| } | ||
| } | ||
| if (consistent) { | ||
| identical.addAll(identicalFiles); | ||
| } else { | ||
| different.addAll(identicalFiles); | ||
| } | ||
| } | ||
| RecoveryDiff recoveryDiff = new RecoveryDiff( | ||
| Collections.unmodifiableList(identical), | ||
| Collections.unmodifiableList(different), | ||
| Collections.unmodifiableList(missing) | ||
| ); | ||
| return recoveryDiff; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the number of files in this snapshot | ||
| */ | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ran this integ test and it still gives an off by 1 error intermittently so I think we should either hold off on it for a separate PR or fix it before merging it in.
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.
That's a good call-out @Poojita-Raj . Yes, I am also able to repro this by running test in interations (fails 3/10). I think this is related to replica lagging behind primary and needs some deep dive. This PR is about avoid shard failures due to delete operations. I will file a new bug to track this.