HDDS-11068. Move SstFiltered flag to a file in the snapshot directory #6965

swamirishi · 2024-07-19T03:41:40Z

What changes were proposed in this pull request?

SstFiltering service directly updates the snapshotInfo and doesn't go through DoubleBufferFlush to update the DB. This interferes with SnapshotInfo updated during SnapshotPurgeRequest.

Consider the case:

S1 <- S2(SstFiltered) <- S3 (Sst unfiltered)

At T1 if S2 is purged and submitted to OM ratis but it hasn't been flushed to the rocksDb but just present in the cache which makes the chain.

S1 <- S3 (Sst Unfiltered in cache)

S2(SstFiltered, deleted in Cache but present in table)

Suppose at T2 SstFiltering service runs on the chain and marks S3 to be SstFiltered which would be directly flushed to DB

S1 <- S3 (Sst filtered in DB)

S1 <- S2 (Present in DB but not present in cache)

Now if the OM is restarted, snapshot chain would have gotten corrupted since both S2 & S3 are pointing S1 in the DB.

Changes made in the patch:

Create an sstFiltered file in the same snapshot directory which will denote the snapshot is filtered and take a snapshot lock while creating this file.
Take a lock while deleting the Snapshot directory since that operation is not atomic.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11068

How was this patch tested?

Unit tests

Change-Id: I9e16fe4b117c232423c1d2de3f48d99acb71827f

Change-Id: I36cee730a30496264b32c74a5f25ae2e92936eba

adoroszlai · 2024-07-19T08:27:15Z

Please wait for clean CI run in fork before opening PR.

Change-Id: If23d180a31439cb2338fc852f8515dd6422aa1a6

Change-Id: Id5e3ae98555b3102e24bdfb3fb38e2e7e24c26a8

swamirishi · 2024-07-19T20:18:56Z

Please wait for clean CI run in fork before opening PR.

Yeah I forgot to switch it to draft.

hemantk-12

Thanks for the patch @swamirishi
Overall patch looks good to me. Left some minor comments and suggestions.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

...ne-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java

hemantk-12 · 2024-07-23T05:48:14Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

-            snapshotLimit--;
-            snapshotFilteredCount.getAndIncrement();
          } catch (RocksDBException | IOException e) {
            LOG.error("Exception encountered while filtering a snapshot", e);


nit: should we log it at an info level, if SstFileringService fails because the snapshot is purged? Otherwise, it will be unnecessary noisy logs.

You cannot be sure why the issue occured. RocksDbException can thrown for many reasons. One of them being the db being closed by another thread.

Yes, that's why I asked to log it at the info level if it fails because snapshot dir doesn't exist which will be the case when it is purged. RocksDB throws exception like org.rocksdb.RocksDBException: While mkdir if missing: path_to_db_dir: No such file or directory..

Change-Id: I438ede5cd999cb991064cf9f4bf142568b03f98f

hemantk-12 · 2024-07-29T19:35:52Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

  private static final int SST_FILTERING_CORE_POOL_SIZE = 1;

+  public static final String SST_FILTERED_FILE = "sstFiltered";
+  private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This directory holds information " +


Suggested change

private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This directory holds information " +

private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This file holds information " +

"if a particular snapshot has filtered out the irrelevant sst files or not.\nDO NOT add, change or delete " +

"this file unless you know what you are doing.\n");

hemantk-12 · 2024-07-29T19:37:00Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

  // multiple times.
  private static final int SST_FILTERING_CORE_POOL_SIZE = 1;

+  public static final String SST_FILTERED_FILE = "sstFiltered";


nit: should we have .txt or any other text file extension?

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

hemantk-12 · 2024-07-29T20:01:55Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

-            snapshotLimit--;
-            snapshotFilteredCount.getAndIncrement();
          } catch (RocksDBException | IOException e) {
            LOG.error("Exception encountered while filtering a snapshot", e);


Yes, that's why I asked to log it at the info level if it fails because snapshot dir doesn't exist which will be the case when it is purged. RocksDB throws exception like org.rocksdb.RocksDBException: While mkdir if missing: path_to_db_dir: No such file or directory..

...ager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java

Change-Id: I373acfefbf65812665794dfeb292a928900b6c73

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

hemantk-12 · 2024-07-31T18:42:57Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java

+  public static final String SST_FILTERED_FILE = "sstFiltered";
+  private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This file holds information " +
+      "if a particular snapshot has filtered out the relevant sst files or not.\nDO NOT add, change or delete " +
+      "any files in this directory unless you know what you are doing.\n");


Suggested change

"any files in this directory unless you know what you are doing.\n");

"this file unless you know what you are doing.\n");

This file is inside the rocksdb directory. The statement is correct right isn't? Basically asking people not to change any file in this directory.

hemantk-12

LGTM

Change-Id: I73029fede1d667b70c68c11527398216e458f8ae

swamirishi · 2024-07-31T23:43:09Z

Thank you for reviewing the patch @hemantk-12

…apache#6965)

…apache#6965) (cherry picked from commit 1ae2701)

…ot directory (apache#6965) (cherry picked from commit 1ae2701) Change-Id: I176724d052cbca8f345d471683320f131222982f

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory

25d178f

Change-Id: I9e16fe4b117c232423c1d2de3f48d99acb71827f

swamirishi requested review from hemantk-12 and smengcl July 19, 2024 03:41

HDDS-11068. Fix checkstyle

b0f7f6b

Change-Id: I36cee730a30496264b32c74a5f25ae2e92936eba

adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jul 19, 2024

swamirishi added 2 commits July 19, 2024 11:38

Merge remote-tracking branch 'apache/master' into HEAD

97649b0

Change-Id: If23d180a31439cb2338fc852f8515dd6422aa1a6

HDDS-11068. Fix tests

71884e0

Change-Id: Id5e3ae98555b3102e24bdfb3fb38e2e7e24c26a8

swamirishi marked this pull request as draft July 19, 2024 19:52

hemantk-12 reviewed Jul 23, 2024

View reviewed changes

HDDS-11068. Address review comments

4723c2e

Change-Id: I438ede5cd999cb991064cf9f4bf142568b03f98f

hemantk-12 reviewed Jul 29, 2024

View reviewed changes

HDDS-11068. Fix comments

d091dbf

Change-Id: I373acfefbf65812665794dfeb292a928900b6c73

hemantk-12 reviewed Jul 31, 2024

View reviewed changes

swamirishi marked this pull request as ready for review July 31, 2024 22:09

hemantk-12 approved these changes Jul 31, 2024

View reviewed changes

HDDS-11068. Fix comments

6423746

Change-Id: I73029fede1d667b70c68c11527398216e458f8ae

swamirishi merged commit 1ae2701 into apache:master Jul 31, 2024

hemantk-12 mentioned this pull request Aug 1, 2024

HDDS-11137. Removed locks from SnapshotPurge and SnapshotSetProperty APIs. #7018

Merged

devabhishekpal pushed a commit to devabhishekpal/ozone that referenced this pull request Aug 8, 2024

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory (…

9c4bf8b

…apache#6965)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Oct 1, 2024

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory (…

542bd10

…apache#6965) (cherry picked from commit 1ae2701)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Oct 1, 2024

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory (…

73b638d

…apache#6965) (cherry picked from commit 1ae2701)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Oct 1, 2024

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory (…

3b15819

…apache#6965) (cherry picked from commit 1ae2701)

xichen01 mentioned this pull request Oct 1, 2024

[DO NOT MERGE] Fix the issue that junit5 tests were not executed in CI #7261

Merged

ptlrs pushed a commit to ptlrs/ozone that referenced this pull request Mar 8, 2025

CDPD-71781. HDDS-11068. Move SstFiltered flag to a file in the snapsh…

2262467

…ot directory (apache#6965) (cherry picked from commit 1ae2701) Change-Id: I176724d052cbca8f345d471683320f131222982f

	"any files in this directory unless you know what you are doing.\n");
	"this file unless you know what you are doing.\n");

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory #6965

HDDS-11068. Move SstFiltered flag to a file in the snapshot directory #6965

Uh oh!

Conversation

swamirishi commented Jul 19, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai commented Jul 19, 2024

Uh oh!

swamirishi commented Jul 19, 2024

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hemantk-12 Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

swamirishi Jul 27, 2024

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hemantk-12 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hemantk-12 Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

swamirishi Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swamirishi Jul 31, 2024 •

edited

Loading