-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-11440. Add a lastTransactionInfo field in SnapshotInfo to check for transactions in flight on the snapshot #7179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
831cd46
4ff6a6e
dc55f37
e556129
b32d074
331acdd
36aed37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| */ | ||
|
|
||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.protobuf.ByteString; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.hadoop.hdds.utils.db.Codec; | ||
| import org.apache.hadoop.hdds.utils.db.CopyObject; | ||
|
|
@@ -124,6 +125,7 @@ public static SnapshotStatus valueOf(SnapshotStatusProto status) { | |
| private long exclusiveSize; | ||
| private long exclusiveReplicatedSize; | ||
| private boolean deepCleanedDeletedDir; | ||
| private ByteString lastTransactionInfo; | ||
|
|
||
| private SnapshotInfo(Builder b) { | ||
| this.snapshotId = b.snapshotId; | ||
|
|
@@ -145,6 +147,7 @@ private SnapshotInfo(Builder b) { | |
| this.exclusiveSize = b.exclusiveSize; | ||
| this.exclusiveReplicatedSize = b.exclusiveReplicatedSize; | ||
| this.deepCleanedDeletedDir = b.deepCleanedDeletedDir; | ||
| this.lastTransactionInfo = b.lastTransactionInfo; | ||
| } | ||
|
|
||
| public void setName(String name) { | ||
|
|
@@ -261,13 +264,15 @@ public SnapshotInfo.Builder toBuilder() { | |
| .setGlobalPreviousSnapshotId(globalPreviousSnapshotId) | ||
| .setSnapshotPath(snapshotPath) | ||
| .setCheckpointDir(checkpointDir) | ||
| .setDbTxSequenceNumber(dbTxSequenceNumber) | ||
| .setDeepClean(deepClean) | ||
| .setSstFiltered(sstFiltered) | ||
| .setReferencedSize(referencedSize) | ||
| .setReferencedReplicatedSize(referencedReplicatedSize) | ||
| .setExclusiveSize(exclusiveSize) | ||
| .setExclusiveReplicatedSize(exclusiveReplicatedSize) | ||
| .setDeepCleanedDeletedDir(deepCleanedDeletedDir); | ||
| .setDeepCleanedDeletedDir(deepCleanedDeletedDir) | ||
| .setLastTransactionInfo(lastTransactionInfo); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the existing snapshots when the system is upgraded ? they would not have this field set.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The lastTransactionInfo would be having a null value |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -293,6 +298,7 @@ public static class Builder { | |
| private long exclusiveSize; | ||
| private long exclusiveReplicatedSize; | ||
| private boolean deepCleanedDeletedDir; | ||
| private ByteString lastTransactionInfo; | ||
|
|
||
| public Builder() { | ||
| // default values | ||
|
|
@@ -411,6 +417,11 @@ public Builder setDeepCleanedDeletedDir(boolean deepCleanedDeletedDir) { | |
| return this; | ||
| } | ||
|
|
||
| public Builder setLastTransactionInfo(ByteString lastTransactionInfo) { | ||
| this.lastTransactionInfo = lastTransactionInfo; | ||
| return this; | ||
| } | ||
|
|
||
| public SnapshotInfo build() { | ||
| Preconditions.checkNotNull(name); | ||
| return new SnapshotInfo(this); | ||
|
|
@@ -445,6 +456,10 @@ public OzoneManagerProtocolProtos.SnapshotInfo getProtobuf() { | |
| sib.setGlobalPreviousSnapshotID(toProtobuf(globalPreviousSnapshotId)); | ||
| } | ||
|
|
||
| if (lastTransactionInfo != null) { | ||
| sib.setLastTransactionInfo(lastTransactionInfo); | ||
| } | ||
|
|
||
| sib.setSnapshotPath(snapshotPath) | ||
| .setCheckpointDir(checkpointDir) | ||
| .setDbTxSequenceNumber(dbTxSequenceNumber) | ||
|
|
@@ -513,6 +528,10 @@ public static SnapshotInfo getFromProtobuf( | |
| snapshotInfoProto.getDeepCleanedDeletedDir()); | ||
| } | ||
|
|
||
| if (snapshotInfoProto.hasLastTransactionInfo()) { | ||
| osib.setLastTransactionInfo(snapshotInfoProto.getLastTransactionInfo()); | ||
| } | ||
|
|
||
| osib.setSnapshotPath(snapshotInfoProto.getSnapshotPath()) | ||
| .setCheckpointDir(snapshotInfoProto.getCheckpointDir()) | ||
| .setDbTxSequenceNumber(snapshotInfoProto.getDbTxSequenceNumber()); | ||
|
|
@@ -605,6 +624,14 @@ public void setDeepCleanedDeletedDir(boolean deepCleanedDeletedDir) { | |
| this.deepCleanedDeletedDir = deepCleanedDeletedDir; | ||
| } | ||
|
|
||
| public ByteString getLastTransactionInfo() { | ||
| return lastTransactionInfo; | ||
| } | ||
|
|
||
| public void setLastTransactionInfo(ByteString lastTransactionInfo) { | ||
| this.lastTransactionInfo = lastTransactionInfo; | ||
| } | ||
|
|
||
| /** | ||
| * Generate default name of snapshot, (used if user doesn't provide one). | ||
| */ | ||
|
|
@@ -673,7 +700,8 @@ public boolean equals(Object o) { | |
| referencedReplicatedSize == that.referencedReplicatedSize && | ||
| exclusiveSize == that.exclusiveSize && | ||
| exclusiveReplicatedSize == that.exclusiveReplicatedSize && | ||
| deepCleanedDeletedDir == that.deepCleanedDeletedDir; | ||
| deepCleanedDeletedDir == that.deepCleanedDeletedDir && | ||
| Objects.equals(lastTransactionInfo, that.lastTransactionInfo); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pre=upgrade snapshots would not have this field set. This can cause equality tests to be fail for such snapshots. Can this equality test cause regression in existing code paths due to this ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would always get the snapshotInfo from the disk. So this value would be null anyways. We don't use the snapshotInfo.equals anywhere in the code flow. It is only in the test case we use this. |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -684,35 +712,15 @@ public int hashCode() { | |
| globalPreviousSnapshotId, snapshotPath, checkpointDir, | ||
| deepClean, sstFiltered, | ||
| referencedSize, referencedReplicatedSize, | ||
| exclusiveSize, exclusiveReplicatedSize, deepCleanedDeletedDir); | ||
| exclusiveSize, exclusiveReplicatedSize, deepCleanedDeletedDir, lastTransactionInfo); | ||
| } | ||
|
|
||
| /** | ||
| * Return a new copy of the object. | ||
| */ | ||
| @Override | ||
| public SnapshotInfo copyObject() { | ||
| return new Builder() | ||
| .setSnapshotId(snapshotId) | ||
| .setName(name) | ||
| .setVolumeName(volumeName) | ||
| .setBucketName(bucketName) | ||
| .setSnapshotStatus(snapshotStatus) | ||
| .setCreationTime(creationTime) | ||
| .setDeletionTime(deletionTime) | ||
| .setPathPreviousSnapshotId(pathPreviousSnapshotId) | ||
| .setGlobalPreviousSnapshotId(globalPreviousSnapshotId) | ||
| .setSnapshotPath(snapshotPath) | ||
| .setCheckpointDir(checkpointDir) | ||
| .setDbTxSequenceNumber(dbTxSequenceNumber) | ||
| .setDeepClean(deepClean) | ||
| .setSstFiltered(sstFiltered) | ||
| .setReferencedSize(referencedSize) | ||
| .setReferencedReplicatedSize(referencedReplicatedSize) | ||
| .setExclusiveSize(exclusiveSize) | ||
| .setExclusiveReplicatedSize(exclusiveReplicatedSize) | ||
| .setDeepCleanedDeletedDir(deepCleanedDeletedDir) | ||
| .build(); | ||
| return this.toBuilder().build(); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -737,6 +745,7 @@ public String toString() { | |
| ", exclusiveSize: '" + exclusiveSize + '\'' + | ||
| ", exclusiveReplicatedSize: '" + exclusiveReplicatedSize + '\'' + | ||
| ", deepCleanedDeletedDir: '" + deepCleanedDeletedDir + '\'' + | ||
| ", lastTransactionInfo: '" + lastTransactionInfo + '\'' + | ||
| '}'; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ | |
| import org.apache.hadoop.hdds.StringUtils; | ||
| import org.apache.hadoop.hdds.conf.OzoneConfiguration; | ||
| import org.apache.hadoop.hdds.server.ServerUtils; | ||
| import org.apache.hadoop.hdds.utils.TransactionInfo; | ||
| import org.apache.hadoop.hdds.utils.db.BatchOperation; | ||
| import org.apache.hadoop.hdds.utils.db.CodecRegistry; | ||
| import org.apache.hadoop.hdds.utils.db.DBCheckpoint; | ||
|
|
@@ -674,6 +675,41 @@ private ReferenceCounted<OmSnapshot> getSnapshot(String snapshotTableKey, boolea | |
| return snapshotCache.get(snapshotInfo.getSnapshotId()); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the last transaction performed on the snapshot has been flushed to disk. | ||
| * @param metadataManager Metadatamanager of Active OM. | ||
| * @param snapshotTableKey table key corresponding to snapshot in snapshotInfoTable. | ||
| * @return True if the changes have been flushed to DB otherwise false | ||
| * @throws IOException | ||
| */ | ||
| public static boolean areSnapshotChangesFlushedToDB(OMMetadataManager metadataManager, String snapshotTableKey) | ||
hemantk-12 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| throws IOException { | ||
| // Need this info from cache since the snapshot could have been updated only on cache and not on disk. | ||
| SnapshotInfo snapshotInfo = metadataManager.getSnapshotInfoTable().get(snapshotTableKey); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we should use SnapshotUtils#getSnapshotInfo. |
||
| return areSnapshotChangesFlushedToDB(metadataManager, snapshotInfo); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the last transaction performed on the snapshot has been flushed to disk. | ||
| * @param metadataManager Metadatamanager of Active OM. | ||
| * @param snapshotInfo SnapshotInfo value. | ||
| * @return True if the changes have been flushed to DB otherwise false. It would return true if the snapshot | ||
| * provided is null meaning the snapshot doesn't exist. | ||
| * @throws IOException | ||
| */ | ||
| public static boolean areSnapshotChangesFlushedToDB(OMMetadataManager metadataManager, SnapshotInfo snapshotInfo) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the usage of this function in PR#7200, these static functions should be moved to SnapshotUtils. |
||
| throws IOException { | ||
| if (snapshotInfo != null) { | ||
swamirishi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| TransactionInfo snapshotTransactionInfo = snapshotInfo.getLastTransactionInfo() != null ? | ||
| TransactionInfo.fromByteString(snapshotInfo.getLastTransactionInfo()) : null; | ||
| TransactionInfo omTransactionInfo = TransactionInfo.readTransactionInfo(metadataManager); | ||
| // If transactionInfo field is null then return true to keep things backward compatible. | ||
| return snapshotTransactionInfo == null || omTransactionInfo.compareTo(snapshotTransactionInfo) >= 0; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Returns OmSnapshot object and skips active check. | ||
| * This should only be used for API calls initiated by background service e.g. purgeKeys, purgeSnapshot, | ||
|
|
||
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.
Why do we need getSkipCache() version for TransactionInfo ?
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.
We are always supposed to get the value from the disk, and never from the cache. There should be no cache present for transactionInfo table. I have added a fail safe check, to skip cache even if it exists.
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 think we agreed that there is no need to skip the cache because we don't update the cache directly for
TransactionInfoTable. Once the transaction gets flushed, it updates the cache as well.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.
Why even check the cache if the cache datastructure is not going to have it. SkipCache would directly go to rocksdb and check it, in contrary to the get method making a roundtrip to cache and then going to rocksdb when not found in cache.