You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sadanand48 (via GitHub)" <gi...@apache.org> on 2023/08/07 14:39:33 UTC
[GitHub] [ozone] sadanand48 opened a new pull request, #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
sadanand48 opened a new pull request, #5155:
URL: https://github.com/apache/ozone/pull/5155
## What changes were proposed in this pull request?
SSTFiltering service should only operate on active snapshots.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9126
## How was this patch tested?
Unit tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287360241
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
+ (isSstFilteringSvcEnabled && snapInfo.isSstFiltered())) {
Review Comment:
! snapInfo.isSstFiltered() ??
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287454406
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
+ (isSstFilteringSvcEnabled && snapInfo.isSstFiltered())) {
Review Comment:
Done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287687156
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
+ SnapshotInfo.SnapshotStatus status, boolean isSstFilteringSvcEnabled,
Review Comment:
nit: unused parameter `SnapshotInfo.SnapshotStatus status`. Please remove it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287508209
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
Review Comment:
@sadanand48 can you please also add/update how your fix is going to solve the problem in the description? Currently it just tells what is the problem.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287487164
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
What is `(isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());` check doing? It is saying that you will try to delete (clean up) a snapshot if SST filtering service is enabled but snapshot is not filtered.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287324077
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -119,6 +123,12 @@ public SnapshotDeletingService(long interval, long serviceTimeout,
this.keyLimitPerSnapshot = conf.getInt(
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK,
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK_DEFAULT);
+ long serviceInterval = ozoneManager.getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ this.isSstFilteringSvcEnabled =
Review Comment:
Done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287876186
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
will handle this in https://issues.apache.org/jira/browse/HDDS-9135
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287694525
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -147,12 +148,12 @@ public BackgroundTaskResult call() throws InterruptedException {
while (iterator.hasNext() && snapshotLimit > 0) {
SnapshotInfo snapInfo = iterator.next().getValue();
- SnapshotInfo.SnapshotStatus snapshotStatus =
- snapInfo.getSnapshotStatus();
+ boolean isSstFilteringServiceEnabled =
+ ((KeyManagerImpl) ozoneManager.getKeyManager())
+ .isSstFilteringSvcEnabled();
// Only Iterate in deleted snapshot
- if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ if (shouldIgnoreSnapshot(snapInfo, isSstFilteringServiceEnabled)) {
Review Comment:
Curious if we need to have end to end test in `TestSnapshotDeletingService` as well or [this](https://github.com/apache/ozone/pull/5155/files#diff-c4c691f6dfa73df833ed29be86d77ef32b206052a5c69febef2c39eaef5fcc51R256) is enough?.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287300924
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -119,6 +123,12 @@ public SnapshotDeletingService(long interval, long serviceTimeout,
this.keyLimitPerSnapshot = conf.getInt(
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK,
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK_DEFAULT);
+ long serviceInterval = ozoneManager.getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ this.isSstFilteringSvcEnabled =
Review Comment:
Sorry it is KeyManagerImpl class not OzoneManager
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287487164
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
What is `(isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());` check doing? It is saying that you will try to clean up a snapshot if SST filtering service is enabled but snapshot is not filtered.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
Review Comment:
nit: use `==` for enum comparison to avoid null checks.
```suggestion
return !snapshotStatus.equals(
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
Review Comment:
Description says "SSTFiltering service should only operate on active snapshots." but here you are skipping the active snapshot check. Which means you will still load snapshot even if it is not ACTIVE. It is not what description says. Please either update the description or remove the `skipActiveCheck`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287481233
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
Review Comment:
nit: use `==` for enum comparison to avoid null check.
```suggestion
return snapshotStatus !=
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287528356
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get()
+ .get(snapshotInfo.getTableKey(), true)) {
Review Comment:
I'll suggested to use `if (snapshotInfo.isSstFiltered() || snapshotInfo.getSnapshotStatus() != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)` at line number 186 instead of loading DELETED snapshot. There is no point in cleaning deleted snapshot.
And add a catch at line 209 to double check that it fails only if snapshot has been deleted.
```
} catch (OMException omException) {
if (snapshotInfo.getSnapshotStatus() ==
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) {
LOG.info("Snapshot with name: '{}', id: '{}' has been deleted.",
snapshotInfo.getName(), snapshotInfo.getSnapshotId());
}
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287528356
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get()
+ .get(snapshotInfo.getTableKey(), true)) {
Review Comment:
I'll suggested to use `if (snapshotInfo.isSstFiltered() || snapshotInfo.getSnapshotStatus() != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)` at line number 186 instead of loading DELETED snapshot. There is no point in cleaning deleted snapshot.
And add a catch at line 209 to double check to fail only if snapshot has been deleted.
```
} catch (OMException omException) {
if (snapshotInfo.getSnapshotStatus() ==
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) {
LOG.info("Snapshot with name: `{}`, id: `{}` has been deleted.",
snapshotInfo.getName(), snapshotInfo.getSnapshotId());
}
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287694525
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -147,12 +148,12 @@ public BackgroundTaskResult call() throws InterruptedException {
while (iterator.hasNext() && snapshotLimit > 0) {
SnapshotInfo snapInfo = iterator.next().getValue();
- SnapshotInfo.SnapshotStatus snapshotStatus =
- snapInfo.getSnapshotStatus();
+ boolean isSstFilteringServiceEnabled =
+ ((KeyManagerImpl) ozoneManager.getKeyManager())
+ .isSstFilteringSvcEnabled();
// Only Iterate in deleted snapshot
- if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ if (shouldIgnoreSnapshot(snapInfo, isSstFilteringServiceEnabled)) {
Review Comment:
Curious if we need to have test for it in `TestSnapshotDeletingService` as well or [this](https://github.com/apache/ozone/pull/5155/files#diff-c4c691f6dfa73df833ed29be86d77ef32b206052a5c69febef2c39eaef5fcc51R256) is enough?.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287263430
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
We can handle this edge case scenario in another PR in the interest of time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287688581
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
Review Comment:
nit: you can add this as an description instead of comment. Something like [this](https://github.com/apache/ozone/blob/7a19afa71401e356cd86a99648e94278ea2850c8/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java#L914). Also if I'm not wrong ordering is incorrect in comment.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286250425
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
It could happen even after doing this check under the lock. We can't hold the lock till the sst filtering service is done with this. Perhaps GC could wait until the snapshot is marked as filtering complete.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670114365
Thanks @hemantk-12
> I don't understand how this is causing OM to go into unhealthy state. We just log [the exception](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L202).
Yeah, I guess the problem is that it is running the task periodically and failing to process this snapshot in a loop due to which OM gets busy. The exact error in this case was the failure to load a deleted snapshot which is fixed now via this patch with skipActiveCheck=true
> I see that there is a problem with the while loop. We should catch exception for individual snapshots and just fail for that particular snapshot otherwise it will block rest of the snapshots SST clean up on-words.
Actually inside the while loop it does iterator.next() so it won't process the same snapshot in the next iteration however it might run in the next time interval of the Filtering task
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287289967
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -170,33 +182,38 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
+ if (snapshotInfo.getSnapshotStatus()
Review Comment:
Oh yes, restricting it here would make the gc not run for unfiltered snapshots. Updated
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287504245
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
Review Comment:
The problem is we are working without a lock here. One wouldn't be able to guage the state of the system when the sstFilteringProcess is going run on the snapshot.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287504451
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
Review Comment:
Done, removed the extra param
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
Review Comment:
Done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1289008645
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
+ SnapshotInfo.SnapshotStatus status, boolean isSstFilteringSvcEnabled,
Review Comment:
It was not used before https://github.com/apache/ozone/pull/5155/commits/318a427a671a4acab9820e6bff6eb21c4dc0cd55.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287688581
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
Review Comment:
nit: you can add this as an description instead of comment. Something like this: https://github.com/apache/ozone/blob/7a19afa71401e356cd86a99648e94278ea2850c8/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java#L914
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287263430
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
We can handle this edge case scenario in another PR, in the interest of time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287487164
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
What is `(isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());` check doing? It is saying that you will try to delete (clean up) a snapshot only if snapshot has been SST filtered if SST filtering service is enabled. SnapshotDeletingService should only delete (try to delete) snapshots which are marked DELETED.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287507470
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
We could land in a race condition where the rocksDB will be open by SSTFilteringService and the OmSnapshotPurgeResponse could delete the checkpoint directory. It would be better to work with only closed rocksdb instances.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287504765
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
Review Comment:
Updated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 closed pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 closed pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
URL: https://github.com/apache/ozone/pull/5155
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670146478
> fails to load or call db.deleteFilesNotMatchingPrefix() for snap3.
This would result in an exception and it would be caught in the below catch block. In the next run of the task which would be after the sst filtering task interval, this would be retried. If we fail due to occurrence of an exception once it will never filter that snapshot
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287791205
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
+ SnapshotInfo.SnapshotStatus status, boolean isSstFilteringSvcEnabled,
+ boolean expectedOutcome) {
+ assertEquals(expectedOutcome,
Review Comment:
```suggestion
snapshotInfo.setSnapshotStatus(status);
assertEquals(expectedOutcome,
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287873003
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -147,12 +148,12 @@ public BackgroundTaskResult call() throws InterruptedException {
while (iterator.hasNext() && snapshotLimit > 0) {
SnapshotInfo snapInfo = iterator.next().getValue();
- SnapshotInfo.SnapshotStatus snapshotStatus =
- snapInfo.getSnapshotStatus();
+ boolean isSstFilteringServiceEnabled =
+ ((KeyManagerImpl) ozoneManager.getKeyManager())
+ .isSstFilteringSvcEnabled();
// Only Iterate in deleted snapshot
- if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ if (shouldIgnoreSnapshot(snapInfo, isSstFilteringServiceEnabled)) {
Review Comment:
Will add this in a follow up jira.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287469786
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
Can you file a Jira for this for post 7.1.9 ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287499442
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
Clean up will only run if the SST Filtering service has been run on the snapshot.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287528356
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get()
+ .get(snapshotInfo.getTableKey(), true)) {
Review Comment:
I'll suggested to use `if (snapshotInfo.isSstFiltered() || snapshotInfo.getSnapshotStatus() != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)` at line number 186 instead of loading DELETED snapshot. There is no point in cleaning deleted snapshot.
And add a catch at line 209 to double check that it fails only if snapshot has been deleted.
```
} catch (OMException omException) {
if (snapshotInfo.getSnapshotStatus() ==
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) {
LOG.info("Snapshot with name: `{}`, id: `{}` has been deleted.",
snapshotInfo.getName(), snapshotInfo.getSnapshotId());
}
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287528356
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get()
+ .get(snapshotInfo.getTableKey(), true)) {
Review Comment:
I'll suggested to use `if (snapshotInfo.isSstFiltered() || snapshotInfo.getSnapshotStatus() != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)` at line number 186 instead of loading DELETED snapshot.
And add a catch at line 209:
```
} catch (OMException omException) {
if (snapshotInfo.getSnapshotStatus() ==
SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) {
LOG.info("Snapshot with name: `{}`, id: `{}` has been deleted.",
snapshotInfo.getName(), snapshotInfo.getSnapshotId());
}
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287493519
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
Review Comment:
Do you need to pass `snapshotStatus` when you are passing the `snapshotInfo` object?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287656384
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get()
+ .get(snapshotInfo.getTableKey(), true)) {
Review Comment:
Done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286232742
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
There is no locking mechanism here. Snapshot could have been marked inactive b/w line 161 & line 194. Failure can still occur. We can probably run the sstFiltering service with skipActiveCheck as True instead.
```suggestion
snapshotInfo.getTableKey(), true)) {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287287906
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
Ok sure.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287475760
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
sure will do.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670489146
Can you address the CI failure ? it looks related.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287242150
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -170,33 +182,38 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
+ if (snapshotInfo.getSnapshotStatus()
Review Comment:
this should not be there.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -152,7 +162,8 @@ public BackgroundTaskResult call() throws InterruptedException {
// Only Iterate in deleted snapshot
if (!snapshotStatus.equals(
- SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)) {
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) ||
Review Comment:
We need a similar check in OMSnapshotPurgeRequest. As in we should process this request only when snapshot is sstFiltered. We can also think of pausing the sst filtering service if it so happens that the snapshot is not sstFiltered but it has made it to the request. This scenario can happen if the request has gone through when sstFiltering service was not running and om restarted with a config change starting the sstFilteringService.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -170,33 +182,38 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
+ if (snapshotInfo.getSnapshotStatus()
Review Comment:
SSTFiltering service should also process inactive snapshots.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde merged pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde merged PR #5155:
URL: https://github.com/apache/ozone/pull/5155
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287487164
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
What is `(isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());` check doing? It is saying that you will try to delete (clean up) a snapshot if SST filtering service is enabled but snapshot is not filtered. SnapshotDeletingService should only delete (try to delete) snapshots which are marked DELETED.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287792367
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
+ SnapshotInfo.SnapshotStatus status, boolean isSstFilteringSvcEnabled,
Review Comment:
Don't remove. This is a required parameter
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286336667
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
With the current patch, I guess even if this exception occurs it will be caught in the below catch block and retried in the next iteration, I think a fix here would be to invalidate the cache entry for this snapshot if the exception occurs and in the next iteration the cache value would be refreshed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287502087
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -181,15 +193,17 @@ public BackgroundTaskResult call() throws Exception {
List<Pair<String, String>> prefixPairs =
constructPrefixPairs(snapshotInfo);
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
Review Comment:
Yeah the decription needs to be updated here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670103422
> I don't understand how this is causing OM to go into unhealthy state. We just log [the exception](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L202).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670137748
> Actually inside the while loop it does iterator.next() so it won't process the same snapshot in the next iteration however it might run in the next time interval of the Filtering task
Not true if it gets stuck to fail a snapshot, loop won't proceed further.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286232742
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
There is no lock here. Snapshot could have been marked inactive b/w line 161 & line 194. Failure can still occur. We can probably run the sstFiltering service with skipActiveCheck as True instead.
```suggestion
snapshotInfo.getTableKey(), true)) {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287691224
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSstFilteringService.java:
##########
@@ -251,6 +252,76 @@ public void testIrrelevantSstFileDeletion()
assertEquals(keysFromActiveDb2, keysFromSnapshot2);
}
+ @Test
+ public void testActiveAndDeletedSnapshotCleanup() throws IOException {
+ RDBStore activeDbStore = (RDBStore) om.getMetadataManager().getStore();
+ String volumeName = "volume1";
+ List<String> bucketNames = Arrays.asList("bucket1", "bucket2");
+
+ // Create 2 Buckets
+ for (String bucketName : bucketNames) {
+ createVolumeAndBucket(volumeName, bucketName);
+ }
+ // Write 25 keys in each bucket, 2 sst files would be generated each for
+ // keys in a single bucket
+ int keyCount = 25;
+ for (int bucketIdx = 0; bucketIdx < bucketNames.size(); bucketIdx++) {
+ for (int i = 1; i <= keyCount; i++) {
+ createKey(writeClient, volumeName, bucketNames.get(bucketIdx),
+ "key" + i);
+ }
+ activeDbStore.getDb().flush(OmMetadataManagerImpl.KEY_TABLE);
+ activeDbStore.getDb().compactRange(OmMetadataManagerImpl.KEY_TABLE);
+ }
+
+ SstFilteringService sstFilteringService =
+ keyManager.getSnapshotSstFilteringService();
+ sstFilteringService.pause();
+
+ writeClient.createSnapshot(volumeName, bucketNames.get(0), "snap1");
+ writeClient.createSnapshot(volumeName, bucketNames.get(0), "snap2");
+
+ SnapshotInfo snapshot1Info = om.getMetadataManager().getSnapshotInfoTable()
+ .get(SnapshotInfo.getTableKey(volumeName, bucketNames.get(0), "snap1"));
+ File snapshot1Dir =
+ new File(OmSnapshotManager.getSnapshotPath(conf, snapshot1Info));
+ SnapshotInfo snapshot2Info = om.getMetadataManager().getSnapshotInfoTable()
+ .get(SnapshotInfo.getTableKey(volumeName, bucketNames.get(0), "snap2"));
+ File snapshot2Dir =
+ new File(OmSnapshotManager.getSnapshotPath(conf, snapshot2Info));
+
+ File snap1Current = new File(snapshot1Dir, "CURRENT");
+ File snap2Current = new File(snapshot2Dir, "CURRENT");
+
+ // wait till both checkpoints are created
+ with().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1))
+ .await().until(() -> snap1Current.exists() && snap2Current.exists());
+
+ long snap1SstFileCountBeforeFilter = Arrays.stream(snapshot1Dir.listFiles())
+ .filter(f -> f.getName().endsWith(".sst")).count();
+ long snap2SstFileCountBeforeFilter = Arrays.stream(snapshot2Dir.listFiles())
+ .filter(f -> f.getName().endsWith(".sst")).count();
+
+ // delete snap1
+ writeClient.deleteSnapshot(volumeName, bucketNames.get(0), "snap1");
+ sstFilteringService.resume();
+ // Filtering service will only act on snap2 as it is an active snaphot
+ with().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofSeconds(1))
+ .await()
+ .until(() -> sstFilteringService.getSnapshotFilteredCount().get() >= 2);
+ long snap1SstFileCountAfterFilter = Arrays.stream(snapshot1Dir.listFiles())
+ .filter(f -> f.getName().endsWith(SST_FILE_EXTENSION)).count();
+ long snap2SstFileCountAfterFilter = Arrays.stream(snapshot2Dir.listFiles())
+ .filter(f -> f.getName().endsWith(SST_FILE_EXTENSION)).count();
+ // one sst will be filtered in both active but not in deleted snapshot
Review Comment:
nit:
```suggestion
// Once SST will be filtered in both active but not in deleted snapshot
// as SstFilteringService won't run on already deleted snapshots but will mark
// it as filtered.
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287688581
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
Review Comment:
nit: you can add this as an description instead of comment. Something like [this](https://github.com/apache/ozone/blob/7a19afa71401e356cd86a99648e94278ea2850c8/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java#L914). Also if I'm not wrong ordering is not correct in comment.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1289008645
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
+ SnapshotInfo.SnapshotStatus status, boolean isSstFilteringSvcEnabled,
Review Comment:
It was not used earlier. https://github.com/apache/ozone/pull/5155/commits/318a427a671a4acab9820e6bff6eb21c4dc0cd55
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287872728
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
Review Comment:
Since I made the test parameterised , it was running the setup again after each combination since @AfterEach is used in TestSnapshotDeletingService.
TestOmSnapshotUtils is lightweight and seemed suitable for testing a single method.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287692809
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
Review Comment:
Why this test is not in `TestSnapshotDeletingService`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287684291
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
Yes, but in that case, SFS will fail for that snapshot.
Created a jira: https://issues.apache.org/jira/browse/HDDS-9135 to revisit the snapshot deletion and filtering synchronization.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287504767
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
Yes, I get that but why do you need that check? If `Snapshot` is marked `DELETED`, we should be able to delete it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287499442
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -564,6 +569,14 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo,
+ SnapshotInfo.SnapshotStatus snapshotStatus,
+ boolean isSstFilteringServiceEnabled) {
+ return !snapshotStatus.equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED)
+ || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered());
Review Comment:
Clean up will only run if the SST Filtering service has been run on the snapshot. This will function will ignore the snapshots.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286259093
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
yeah this should solve the issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1286335242
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -149,49 +161,55 @@ public BackgroundTaskResult call() throws Exception {
Table.KeyValue<String, SnapshotInfo> keyValue = iterator.next();
String snapShotTableKey = keyValue.getKey();
SnapshotInfo snapshotInfo = keyValue.getValue();
- UUID snapshotId = snapshotInfo.getSnapshotId();
-
- File omMetadataDir =
- OMStorage.getOmDbDir(ozoneManager.getConfiguration());
- String snapshotDir = omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- Path filePath =
- Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
-
- // If entry for the snapshotID is present in this file,
- // it has already undergone filtering.
- if (Files.exists(filePath)) {
- List<String> processedSnapshotIds = Files.readAllLines(filePath);
- if (snapshotId != null &&
- processedSnapshotIds.contains(snapshotId.toString())) {
- continue;
+ if (snapshotInfo.getSnapshotStatus()
+ .equals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ UUID snapshotId = snapshotInfo.getSnapshotId();
+
+ File omMetadataDir =
+ OMStorage.getOmDbDir(ozoneManager.getConfiguration());
+ String snapshotDir =
+ omMetadataDir + OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
+ Path filePath =
+ Paths.get(snapshotDir + OM_KEY_PREFIX + FILTERED_SNAPSHOTS);
+
+ // If entry for the snapshotID is present in this file,
+ // it has already undergone filtering.
+ if (Files.exists(filePath)) {
+ List<String> processedSnapshotIds = Files.readAllLines(filePath);
+ if (snapshotId != null && processedSnapshotIds.contains(
+ snapshotId.toString())) {
+ continue;
+ }
}
- }
- LOG.debug("Processing snapshot {} to filter relevant SST Files",
- snapShotTableKey);
-
- List<Pair<String, String>> prefixPairs =
- constructPrefixPairs(snapshotInfo);
-
- try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
- snapshotMetadataReader = snapshotCache.get()
- .get(snapshotInfo.getTableKey())) {
- OmSnapshot omSnapshot = (OmSnapshot) snapshotMetadataReader.get();
- RDBStore rdbStore = (RDBStore) omSnapshot.getMetadataManager()
- .getStore();
- RocksDatabase db = rdbStore.getDb();
- try (BootstrapStateHandler.Lock lock =
- getBootstrapStateLock().lock()) {
- db.deleteFilesNotMatchingPrefix(prefixPairs, FILTER_FUNCTION);
+ LOG.debug("Processing snapshot {} to filter relevant SST Files",
+ snapShotTableKey);
+
+ List<Pair<String, String>> prefixPairs =
+ constructPrefixPairs(snapshotInfo);
+
+ try (
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ snapshotMetadataReader = snapshotCache.get().get(
+ snapshotInfo.getTableKey())) {
Review Comment:
There is no field in the snapshot table to check status of filtering complete. We rely on the 'filtered snapshots' file in the snapshots directory, Even if we update the file with the snapshot name as part of the snapshot delete , there is still a possibility that this file was opened by the filtering service before this update was made during delete and we could still get the same issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on PR #5155:
URL: https://github.com/apache/ozone/pull/5155#issuecomment-1670610853
Thank you @sadanand48 for the PR and @hemantk-12 @swamirishi for the reviews.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService throws Exception while processing deleted snapshots.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1288847350
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotUtils.java:
##########
@@ -78,4 +84,41 @@ public void testLinkFiles(@TempDir File tempDir) throws Exception {
assertEquals(tree1Files, tree2Files);
GenericTestUtils.deleteDirectory(tempDir);
}
+
+
+ private static Stream<Arguments> testCasesForIgnoreSnapshotGc() {
+ SnapshotInfo filteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build();
+ SnapshotInfo unFilteredSnapshot =
+ SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1")
+ .build();
+ // {IsSnapshotFiltered,isSnapshotDeleted,IsSstServiceEnabled = ShouldIgnore}
+ return Stream.of(Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false),
+ Arguments.of(unFilteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true),
+ Arguments.of(filteredSnapshot,
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testCasesForIgnoreSnapshotGc")
+ public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo,
Review Comment:
Better place to add this test would have been https://github.com/apache/ozone/tree/12f3f5aeb3660591f9874ae0200ddd21a06a2aa9/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #5155: HDDS-9126. [Snapshot] SstFilteringService should only process active snapshots.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5155:
URL: https://github.com/apache/ozone/pull/5155#discussion_r1287298613
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -119,6 +123,12 @@ public SnapshotDeletingService(long interval, long serviceTimeout,
this.keyLimitPerSnapshot = conf.getInt(
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK,
OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK_DEFAULT);
+ long serviceInterval = ozoneManager.getConfiguration()
+ .getTimeDuration(OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL,
+ OZONE_SNAPSHOT_SST_FILTERING_SERVICE_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ this.isSstFilteringSvcEnabled =
Review Comment:
@sadanand48 nit: We have the same logic in OzoneManager class. We can have this logic there instead.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org