Writable warm relocation recovery #14670
Conversation
Signed-off-by: Nishant Goel <[email protected]>
Signed-off-by: Nishant Goel <[email protected]>
github-actions
bot
added
enhancement
Signed-off-by: Nishant Goel <[email protected]>
Signed-off-by: Nishant Goel <[email protected]>
|
❌ Gradle check result for 41fd1ec: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 3004994: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for d42263b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
nisgoel-amazon
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
| @@ -171,7 +171,9 @@ public synchronized void updateSegments(final SegmentInfos infos) throws IOExcep | |||
| // a lower gen from a newly elected primary shard that is behind this shard's last commit gen. | |||
| // In that case we still commit into the next local generation. | |||
| if (incomingGeneration != this.lastReceivedPrimaryGen) { | |||
| flush(false, true); | |||
| if (engineConfig.getIndexSettings().isStoreLocalityPartial() == false) { | |||
There was a problem hiding this comment.
this engine will only flush here and on close. I don't think we have a reason to commit on close as we will sync from store on re-open? If thats the case lets push this logic to the flush method call and cover both cases? Please also add a comment to why we are not committing here.
| @@ -5052,7 +5054,7 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, final Runn | |||
| storeDirectory = new StoreRecovery.StatsDirectoryWrapper(store.directory(), recoveryState.getIndex()); | |||
| for (String file : uploadedSegments.keySet()) { | |||
| long checksum = Long.parseLong(uploadedSegments.get(file).getChecksum()); | |||
| if (overrideLocal || localDirectoryContains(storeDirectory, file, checksum) == false) { | |||
| if (shouldOverrideLocalFiles || localDirectoryContains(storeDirectory, file, checksum) == false) { | |||
There was a problem hiding this comment.
Rather than adding all these checks to this method on locality can we create a separate trimmed down sync method for this case?
| @@ -117,9 +117,14 @@ public void getSegmentFiles( | |||
| final List<String> toDownloadSegmentNames = new ArrayList<>(); | |||
| for (StoreFileMetadata fileMetadata : filesToFetch) { | |||
| String file = fileMetadata.name(); | |||
| assert directoryFiles.contains(file) == false : "Local store already contains the file " + file; | |||
| assert directoryFiles.contains(file) == false || indexShard.indexSettings().isStoreLocalityPartial() | |||
There was a problem hiding this comment.
Can we not skip the call to getSegmentFiles entirely? That would greatly reduce the changes to SegmentReplicationTarget.
| cancellableThreads.checkForCancel(); | ||
| state.setStage(SegmentReplicationState.Stage.FILE_DIFF); | ||
| final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), indexShard.getSegmentMetadataMap()); | ||
| final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), finalReplicaMd); |
There was a problem hiding this comment.
I don't think we need to compute a diff here if all files are remote and simply return early if data locality is partial. The Checkpoint info call will include the sis bytes which we load onto the readermanager.
| verifyStoreContent(); | ||
| // skipping verify store content over here as readLastCommittedSegmentsInfo files are not present in latest metadata of remote | ||
| // store. | ||
| if (!warmIndexSegmentReplicationEnabled()) { |
There was a problem hiding this comment.
i know similar logic was implemented with remote store but I would prefer we not couple these test classes together and instead write new tests where appropriate for warm cases. Or, we break up the SegmentReplicationIT such that reusable tests move to the base IT.
|
|
||
| @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) | ||
| @ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) | ||
| public class WarmIndexRemoteStoreSegmentReplicationIT extends SegmentReplicationIT { |
There was a problem hiding this comment.
there are likely tests in the regular segrep suite that aren't valid here. Particularly when asserting against store content.
| // skipping verify store content over here as readLastCommittedSegmentsInfo files are not present in latest metadata of remote | ||
| // store. | ||
| if (!warmIndexSegmentReplicationEnabled()) { | ||
| verifyStoreContent(); |
There was a problem hiding this comment.
Can we add a backlog issue to fix this?
|
This PR is stalled because it has been open for 30 days with no activity. |
Description
Changes are related to warm index recovery and relocation flow. In this change we are not downloading the complete files in replicas.
Related Issues
Resolves #13647
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.