shekhars-li commented on code in PR #1676:
URL: https://github.com/apache/samza/pull/1676#discussion_r1286266903
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/BlobStoreRestoreManager.java:
##########
@@ -254,8 +263,10 @@ static CompletableFuture<Void> restoreStores(String
jobName, String jobId, TaskN
throw new SamzaException(String.format("Error deleting store
directory: %s", storeDir), e);
}
+ // If getDeletedBlobs is enabled - always restore so that we get all the
blobs, including the deleted blobs,
+ // immediately restore it locally and backup to create new checkpoint.
boolean shouldRestore = shouldRestore(taskName.getTaskName(), storeName,
dirIndex,
- storeCheckpointDir, storageConfig, dirDiffUtil);
+ storeCheckpointDir, storageConfig, dirDiffUtil) || getDeletedBlobs;
Review Comment:
> Any scenarios where shouldRestore returns false but we want to force
restore?
Yes.
When there is no diff between remote and local snapshot, shouldRestore could
return false. However, if the SnapshotIndex was deleted, we want to force
restore everything (and then backup, create new checkpoint etc). This is
because init() delegates this to restore and only gets the snapshotIndex with
getDeleted. It does not actually create a new copy of everything.
##########
samza-core/src/main/java/org/apache/samza/storage/blobstore/BlobStoreRestoreManager.java:
##########
@@ -254,8 +263,10 @@ static CompletableFuture<Void> restoreStores(String
jobName, String jobId, TaskN
throw new SamzaException(String.format("Error deleting store
directory: %s", storeDir), e);
}
+ // If getDeletedBlobs is enabled - always restore so that we get all the
blobs, including the deleted blobs,
+ // immediately restore it locally and backup to create new checkpoint.
boolean shouldRestore = shouldRestore(taskName.getTaskName(), storeName,
dirIndex,
- storeCheckpointDir, storageConfig, dirDiffUtil);
+ storeCheckpointDir, storageConfig, dirDiffUtil) || getDeletedBlobs;
Review Comment:
> Any scenarios where shouldRestore returns false but we want to force
restore?
Yes.
When there is no diff between remote and local snapshot, shouldRestore could
return false. However, if the SnapshotIndex was deleted, we want to force
restore everything (and then backup, create new checkpoint etc). This is
because init() delegates this to restore and only gets the snapshotIndex with
getDeleted. It does not actually create a new copy of everything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
