HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files #4376

swamirishi · 2023-03-10T18:20:38Z

What changes were proposed in this pull request?

Currently inorder to figure out the deletion, the SST file for from snapshot is also read. With changes in HDDS-8028 tombstone entries in sst file can be read & thus can be used to read the deleted keys. Thus can be used as a hint to get the deleted key entry.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit Tests

adoroszlai · 2023-03-10T20:06:34Z

This is based on top of commits that belong to another open PR, so this should be marked as draft:

to avoid someone accidentally merging this one before the other
to save CI resources, since this one will need to be updated after the other PR gets merged

smengcl · 2023-03-15T20:48:12Z

@hemantk-12 Would you take a look at this?

hadoop-hdds/common/src/main/resources/ozone-default.xml

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

hemantk-12

Thanks for the patch @swamirishi.

Making thinks generic is a good idea but it is not needed always (YAGNI). In this patch, you tried to make ManagedSstFileIterator and MultipleSstFileIterator generic. Which is not a bad idea if they were generic enough. Both of them are internal to ManagedSstFileReader and have defined type String. Making these generic is just making the code complex, instead of simplifying it. It would be great if you could simplify it.

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

...cks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

smengcl · 2023-03-22T00:01:28Z

@swamirishi Can you resolve the comments that are addressed?

hemantk-12

I don't know if you missed my general comment on Making thinks generic . It would be great if you could reply to that.

hemantk-12 · 2023-03-22T19:53:46Z

...cks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java

IMO, the whole comment is unnecessary. @Override will get the Javadoc comment from iterator interface. There is no needed to specifying the same thing. Talking about the RTE, anything can cause that and stating that doesn't make much difference.

hemantk-12 · 2023-03-22T19:58:37Z

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

I meant something gets added in between definition and closing which can cause some exception, there will be a memory leak. It is ideal to define items closer to where are used.

ManagedReadOptions to declare variable as well.

ManagedReadOptions readOptions = new ManagedReadOptions();

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

swamirishi · 2023-03-24T18:18:57Z

Thanks for the patch @swamirishi.

Making thinks generic is a good idea but it is not needed always (YAGNI). In this patch, you tried to make ManagedSstFileIterator and MultipleSstFileIterator generic. Which is not a bad idea if they were generic enough. Both of them are internal to ManagedSstFileReader and have defined type String. Making these generic is just making the code complex, instead of simplifying it. It would be great if you could simplify it.

I actually made it generic, currently we are using the sst files & the tombstone entries as mere hints. But when we have to implement the optimal snapdiff calculation just using the delta files then we need other info & the just the key wouldn't be sufficient we would require the the objectId & the seqId from the SST file as well. That is why I made it generic.

prashantpogde · 2023-03-27T21:12:01Z

@hemantk-12 @swamirishi Can we mark all the comments that are resolved here ?

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CloseableIterator.java

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

prashantpogde

LGTM. We need to add some tests to validate these code paths. The tests can go in a separate PR.

smengcl · 2023-04-04T23:55:43Z

Spotted a SnapDiff robot test failure in CI:

Snapshot Diff                                                         | FAIL |
key-rnpgd: 'Snapshot diff job is IN_PROGRESS
Please retry after 1000 ms.' does not contain '+'
------------------------------------------------------------------------------
Read Snapshot                                                         | PASS |
------------------------------------------------------------------------------
Snapshot.Snapshot-Sh :: Test for using sh commands with snapshots.    | FAIL |
4 tests, 3 passed, 1 failed
==============================================================================
Snapshot                                                              | FAIL |
12 tests, 6 passed, 1 failed, 5 skipped

Could you double check to make sure this is unrelated? File a jira if it is just a flaky test. @swamirishi

hemantk-12

Approved with some minor comments.

hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java

...cks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java

smengcl · 2023-04-14T16:05:59Z

CI is failing repeatedly. @swamirishi can you take a look:

https://github.com/apache/ozone/actions/runs/4693597042/jobs/8323475114

https://github.com/apache/ozone/actions/runs/4693597042/jobs/8323476655

swamirishi · 2023-04-18T14:46:48Z

@smengcl Can this be merged?

smengcl · 2023-04-18T20:19:15Z

@smengcl Can this be merged?

CI passed. nice. Will merge this.

* master: (440 commits) HDDS-8445. Move PlacementPolicy back to SCM (apache#4588) HDDS-8335. ReplicationManager: EC Mis and Under replication handlers should handle overloaded exceptions (apache#4593) HDDS-8355. Intermittent failure in TestOMRatisSnapshots#testInstallSnapshot (apache#4592) HDDS-8444. Increase timeout of CI build (apache#4586) HDDS-8446. Selective checks: handle change in ci.yaml (apache#4587) HDDS-8440. Ozone Manager crashed with ClassCastException when deleting FSO bucket. (apache#4582) HDDS-7309. Enable by default GRPC between S3G and OM (apache#3820) HDDS-8458. Mark TestBlockDeletion#testBlockDeletion as flaky HDDS-8385. Ozone can't process snapshot when service UID > 2097151 (apache#4580) HDDS-8424: Preserve legacy bucket getKeyInfo behavior (apache#4576) HDDS-8453. Mark TestDirectoryDeletingServiceWithFSO#testDirDeletedTableCleanUpForSnapshot as flaky HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files (apache#4376) HDDS-8270. Measure checkAccess latency for Ozone objects (apache#4467) HDDS-8109. Seperate Ratis and EC MisReplication Handling (apache#4577) HDDS-8429. Checkpoint is not closed properly in OMDBCheckpointServlet (apache#4575) HDDS-8253. Set ozone.metadata.dirs to temporary dir if not defined in S3 Gateway (apache#4455) HDDS-8400. Expose rocksdb last sequence number through metrics (apache#4557) HDDS-8333. ReplicationManager: Allow partial EC reconstruction if insufficient nodes available (apache#4579) HDDS-8147. Introduce latency metrics for S3 Gateway operations (apache#4383) HDDS-7908. Support OM Metadata operation Generator in `Ozone freon` (apache#4251) ...

adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Mar 10, 2023

adoroszlai marked this pull request as draft March 10, 2023 20:06

swamirishi force-pushed the HDDS-8137 branch from 81ee4d2 to 7f60dfc Compare March 13, 2023 02:06

swamirishi marked this pull request as ready for review March 13, 2023 15:34

swamirishi changed the title ~~HDDS-8137. Snapdiff to use tombstone entries in SST file~~ HDDS-8137. Snapdiff to use tombstone entries in SST file Mar 13, 2023

smengcl reviewed Mar 16, 2023

View reviewed changes

hadoop-hdds/common/src/main/resources/ozone-default.xml Outdated Show resolved Hide resolved

smengcl reviewed Mar 16, 2023

View reviewed changes

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java Outdated Show resolved Hide resolved

smengcl reviewed Mar 16, 2023

View reviewed changes

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java Outdated Show resolved Hide resolved

smengcl reviewed Mar 16, 2023

View reviewed changes

...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java Outdated Show resolved Hide resolved

hemantk-12 requested changes Mar 17, 2023

View reviewed changes

smengcl changed the title ~~HDDS-8137. Snapdiff to use tombstone entries in SST file~~ HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files Mar 17, 2023

smengcl reviewed Mar 17, 2023

View reviewed changes

smengcl mentioned this pull request Mar 18, 2023

HDDS-6064. Print proper JSON from ozone debug ldb scan #4420

Merged

1 task

hemantk-12 reviewed Mar 22, 2023

View reviewed changes

prashantpogde self-requested a review March 23, 2023 20:38

swamirishi force-pushed the HDDS-8137 branch from 22ab1b1 to ca92cff Compare March 29, 2023 21:53

hemantk-12 reviewed Mar 29, 2023

View reviewed changes

hemantk-12 reviewed Mar 31, 2023

View reviewed changes

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java Outdated Show resolved Hide resolved

prashantpogde approved these changes Apr 4, 2023

View reviewed changes

smengcl approved these changes Apr 4, 2023

View reviewed changes

hemantk-12 approved these changes Apr 5, 2023

View reviewed changes

swamirishi added 2 commits April 15, 2023 17:20

HDDS-8137: Snapdiff to use tombstone entries in SST file

f65fc1d

HDDS-8137: Fix checkstyle issue

cff1971

swamirishi added 10 commits April 15, 2023 17:20

HDDS-8137: Remove redundant dependency

9874613

HDDS-8137: Address review comments

ca045d4

HDDS-8137: Fix checkstyle issue

8b90667

HDDS-8137: Add property to ozone-default

2bba6b0

HDDS-8137: Add line

9af68a8

HDDS-8137. Address review comments

6df1838

HDDS-8137. Address review comments

3426d68

HDDS-8137. Address review comments

fbd4455

HDDS-8137. Address review comments

8c0d728

HDDS-8137. Fix indentation

6a608d0

swamirishi force-pushed the HDDS-8137 branch from 6e76635 to 6a608d0 Compare April 16, 2023 00:21

HDDS-8137. Fix null pointer exception

d40c908

swamirishi force-pushed the HDDS-8137 branch from e239ce3 to d40c908 Compare April 16, 2023 13:44

HDDS-8137. Add init function

5d73f1f

smengcl merged commit 8f745fc into apache:master Apr 18, 2023

HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files #4376

HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files #4376

Uh oh!

Conversation

swamirishi commented Mar 10, 2023

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 Mar 10, 2023

Uh oh!

smengcl commented Mar 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hemantk-12 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smengcl commented Mar 22, 2023

Uh oh!

hemantk-12 left a comment • 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 Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swamirishi commented Mar 24, 2023

Uh oh!

prashantpogde commented Mar 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prashantpogde left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented Apr 4, 2023

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smengcl commented Apr 14, 2023

Uh oh!

swamirishi commented Apr 18, 2023

Uh oh!

hemantk-12 left a comment •

edited

Loading

hemantk-12 left a comment •

edited

Loading