-
Notifications
You must be signed in to change notification settings - Fork 514
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
HDDS-11411. Snapshot garbage collection should not run when the keys are moved from a deleted snapshot to the next snapshot in the chain #7193
Changes from all commits
b5f21a0
7c556b4
bad7fe6
7e66143
83491aa
05787df
bf40940
7b570d5
89ca0f3
2a707fa
792db8e
36411a6
723c52b
3f568db
7cb49b7
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 |
---|---|---|
|
@@ -1381,6 +1381,8 @@ message PurgeKeysRequest { | |
// if set, will purge keys in a snapshot DB instead of active DB | ||
optional string snapshotTableKey = 2; | ||
repeated SnapshotMoveKeyInfos keysToUpdate = 3; | ||
// previous snapshotID can also be null & this field would be absent in older requests. | ||
optional NullableUUID expectedPreviousSnapshotID = 4; | ||
} | ||
|
||
message PurgeKeysResponse { | ||
|
@@ -1403,6 +1405,12 @@ message PurgePathsResponse { | |
message PurgeDirectoriesRequest { | ||
repeated PurgePathRequest deletedPath = 1; | ||
optional string snapshotTableKey = 2; | ||
// previous snapshotID can also be null & this field would be absent in older requests. | ||
optional NullableUUID expectedPreviousSnapshotID = 3; | ||
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. same comment as above regarding this field being null. |
||
} | ||
|
||
message NullableUUID { | ||
hemantk-12 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
optional hadoop.hdds.UUID uuid = 1; | ||
} | ||
|
||
message PurgeDirectoriesResponse { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,6 +143,8 @@ | |
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_INTERVAL_DEFAULT; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_TIMEOUT; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_OPEN_KEY_CLEANUP_SERVICE_TIMEOUT_DEFAULT; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED_DEFAULT; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL_DEFAULT; | ||
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_SNAPSHOT_DIRECTORY_SERVICE_TIMEOUT; | ||
|
@@ -230,6 +232,8 @@ public KeyManagerImpl(OzoneManager om, ScmClient scmClient, | |
|
||
@Override | ||
public void start(OzoneConfiguration configuration) { | ||
boolean isSnapshotDeepCleaningEnabled = configuration.getBoolean(OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED, | ||
OZONE_SNAPSHOT_DEEP_CLEANING_ENABLED_DEFAULT); | ||
if (keyDeletingService == null) { | ||
long blockDeleteInterval = configuration.getTimeDuration( | ||
OZONE_BLOCK_DELETING_SERVICE_INTERVAL, | ||
|
@@ -241,7 +245,7 @@ public void start(OzoneConfiguration configuration) { | |
TimeUnit.MILLISECONDS); | ||
keyDeletingService = new KeyDeletingService(ozoneManager, | ||
scmClient.getBlockClient(), this, blockDeleteInterval, | ||
serviceTimeout, configuration); | ||
serviceTimeout, configuration, isSnapshotDeepCleaningEnabled); | ||
keyDeletingService.start(); | ||
} | ||
|
||
|
@@ -314,7 +318,7 @@ public void start(OzoneConfiguration configuration) { | |
} | ||
} | ||
|
||
if (snapshotDirectoryCleaningService == null && | ||
if (isSnapshotDeepCleaningEnabled && snapshotDirectoryCleaningService == null && | ||
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. && or 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. It has to be && . Configuration to isSnapshotDeepCleaningEnabled should be enabled & also snapshotDirectoryCleaningService should not have been initialized before. |
||
ozoneManager.isFilesystemSnapshotEnabled()) { | ||
long dirDeleteInterval = configuration.getTimeDuration( | ||
OZONE_SNAPSHOT_DIRECTORY_SERVICE_INTERVAL, | ||
|
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.
How do you deal with it if this field is null ?
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.
protobuf fields cannot be null.
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 want to pass expectedPreviousSnapshotID = null for the case there are no snapshots in the chain. But older requests might not have a expectedPreviousSnapshotID in the request, so this validation could incorrectly run for the older requests leading to inconsistencies in the OM db on replays.
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.
Since there is no direct explicit way to differentiate b/w older requests & null values. I had to create a wrapper which means I can set NullableUUID which doesn't have anything inside. Since uuid field inside NullableUUID type is optional, we can signify this as a newer request and having nothing inside the field would signify a null value.