HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator #6182

swamirishi · 2024-02-07T00:09:57Z

What changes were proposed in this pull request?

Including rocksdb in ozone_rocks_tools native library interferes with rocksdb lib loaded from rocksdbjni jar, which leads to issues like block checksum mismatch error. Bad file descriptor error. Instead of adding rocksdb static lib as static lib to ozone_rocks_tools adding rocksdb static lib as a shared lib should prevent such issues from occuring.
Changed the rocks tools build to use rocksdbjnlllib from rocksdb jar instead of building rocksdb from scratch.
Implemented RawSSTFileReader & RawSSTFileReaderIterator in rocksdb tools to implement an iterator in the native side to read tombstone entries.

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

How was this patch tested?

Refactored and changed existing unit testcases and integration tests.

…ibrary

…tools

This reverts commit f4aa7ec.

# Conflicts: # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java

swamirishi · 2024-02-15T17:33:39Z

The PR can be reviewed now. I have fixed all the issues with the patch.

hemantk-12

Thanks @swamirishi for working on this.

...e/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReaderIterator.java

...ks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReader.java

hemantk-12 · 2024-02-21T00:29:24Z

...ks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReader.java

+  public static boolean loadLibrary() throws NativeLibraryNotLoadedException {
+    ManagedRocksObjectUtils.loadRocksDBLibrary();
+    if (!NativeLibraryLoader.getInstance()
+        .loadLibrary(ROCKS_TOOLS_NATIVE_LIBRARY_NAME)) {
+      throw new NativeLibraryNotLoadedException(
+          ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+    }
+    return true;
+  }


nit:

I think, this code should be moved to SnapshotDiffManager#initSSTDumpTool(). And in test should load lib wherever is needed in @BeforeAll. ManagedRawSSTFileReader class in itself doesn't care if native lib is loaded or not. And rely on the caller that they should load it before using it.

May be call loadNativeLib rather than initSSTDumpTool.

loadLibrary is still in ManagedRawSstFileReader. Please move it to SnapshotDiffManager.

It is responsibility of the SnapshotDiffManager to make sure native-lib is loaded before using optimized SnapDiff.

No I believe this particular piece should reside in this particular module since this class cannot run without the library being loaded. It has to be self contained.

Even rocksdb does something similar

I disagree with you because if ManagedRawSstFileReader has some dependency on NativeLibrary, it should be injected to it.
Since it is static loading, we can move this code inside the constructor of ManagedRawSstFileReader or static block (which I don't think possible because it is needed to be loaded based on the config) but loading this way doesn't feel good to me.

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

...ks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReader.java

...e/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReaderIterator.java

...ks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRawSSTFileReader.java

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

hadoop-hdds/rocks-native/src/main/patches/rocks-native.patch

hadoop-hdds/rocks-native/pom.xml

…erator & reader

hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/NativeLibraryLoader.java

pom.xml

hadoop-hdds/rocks-native/src/CMakeLists.txt

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

hemantk-12

Overall looks good to me.

There are few cosmetics comments which are not addressed. Please create follow up task if you don't intend to fix them as part of this PR.

swamirishi · 2024-02-22T22:23:36Z

Thanks for the review @hemantk-12 and @smengcl

smengcl · 2024-02-22T23:53:47Z

Thanks @swamirishi for the fix and improvement.

…leReaderIterator (apache#6182) (cherry picked from commit d883d7d) Change-Id: I5cade05d6be565acc3d93b6376e7ecf679dccf55

…ator (apache#6182) (cherry picked from commit d883d7d)

swamirishi added 3 commits February 6, 2024 15:41

HDDS-10149. Fix native lib to not include rocksdb library as static l…

4d60dc7

…ibrary

HDDS-10149. Fix checkstyle

2dc7c0b

HDDS-10149. Test

824678d

swamirishi marked this pull request as ready for review February 14, 2024 02:34

swamirishi changed the title ~~HDDS-10149. Fix native lib to not include rocksdb library as static library~~ HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator Feb 14, 2024

swamirishi requested review from hemantk-12, aswinshakil, smengcl and adoroszlai February 14, 2024 02:39

HDDS-10149. New JNI layer containing new rawSstFileReader on rocksdb …

f2a7869

…tools

swamirishi force-pushed the HDDS-10149 branch from f1fc2c7 to f2a7869 Compare February 14, 2024 02:50

swamirishi added 4 commits February 13, 2024 18:55

HDDS-10149. Remove usage of managed sst dump tool

f25a570

HDDS-10149. Fix findbugs & rat check

a11705a

HDDS-10149. Fix findbugs

c80234d

HDDS-10149. Fix checkstyle

406db5b

adoroszlai marked this pull request as draft February 14, 2024 08:33

swamirishi added 5 commits February 14, 2024 09:08

HDDS-10149. Fix cpp shadow error

41d557d

HDDS-10149. Fix native

f9dcb2e

HDDS-10149. Fix native

194c8af

HDDS-10149. Fix native

09e640c

HDDS-10149. Debug native failure

bbb15ad

swamirishi force-pushed the HDDS-10149 branch from 4cb44ec to bbb15ad Compare February 14, 2024 20:08

swamirishi added 8 commits February 14, 2024 12:35

HDDS-10149. Debug native failure

dd78c1e

HDDS-10149. Fix test cases

2d2c8f6

HDDS-10149. Remove debug flag

be7faa3

HDDS-10149. Adding log

5a64230

HDDS-10149. Fix bug

f4aa7ec

HDDS-10149. Add log

0e1f50d

HDDS-10149. Add ssh session

209c258

Revert "HDDS-10149. Fix bug"

2e08516

This reverts commit f4aa7ec.

swamirishi added 3 commits February 15, 2024 08:44

HDDS-10149. Unmark unhealthy snapshots native tests

66921e9

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

8cc3df4

# Conflicts: # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java

HDDS-10149. Fix checkstyle

c0752e1

swamirishi marked this pull request as ready for review February 15, 2024 17:32

HDDS-10149. Fix testcase

da80339

hemantk-12 reviewed Feb 21, 2024

View reviewed changes

hadoop-hdds/rocks-native/src/main/patches/rocks-native.patch Show resolved Hide resolved

hemantk-12 reviewed Feb 22, 2024

View reviewed changes

hadoop-hdds/rocks-native/src/main/patches/rocks-native.patch Outdated Show resolved Hide resolved

hadoop-hdds/rocks-native/pom.xml Outdated Show resolved Hide resolved

swamirishi added 5 commits February 21, 2024 16:34

HDDS-10149. Address review comments

03f22f9

HDDS-10149. Fix checkstyle within patch file

2d70537

HDDS-10149. Remove unwanted classes and tools from rocks tools

4c3890b

HDDS-10149. Remove unwanted classes and tools from rocks tools

8a1b0bb

HDDS-10149. Change Function definitions & imports for raw_sst_file_it…

1ab1bc6

…erator & reader

smengcl reviewed Feb 22, 2024

View reviewed changes

hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/NativeLibraryLoader.java Outdated Show resolved Hide resolved

smengcl reviewed Feb 22, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

HDDS-10149. Address review comments

a36487b

smengcl reviewed Feb 22, 2024

View reviewed changes

hadoop-hdds/rocks-native/src/CMakeLists.txt Show resolved Hide resolved

...one/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java Outdated Show resolved Hide resolved

HDDS-10149. Address review comments

9b48779

hemantk-12 approved these changes Feb 22, 2024

View reviewed changes

swamirishi mentioned this pull request Feb 22, 2024

Raw SST File Iterator & reader facebook/rocksdb#12370

Closed

HDDS-10149. Address review comments

c4ee8c8

hemantk-12 approved these changes Feb 22, 2024

View reviewed changes

swamirishi merged commit d883d7d into apache:master Feb 22, 2024
37 checks passed

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 26, 2024

HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIter…

2f63c53

…ator (apache#6182) (cherry picked from commit d883d7d)

xichen01 mentioned this pull request Jul 27, 2024

[DO NOT MERGE] Backport some fixes from master to ozone-1.4 #6998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator #6182

HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator #6182

swamirishi commented Feb 7, 2024 •

edited

Loading

swamirishi commented Feb 15, 2024

hemantk-12 left a comment

hemantk-12 Feb 21, 2024

swamirishi Feb 22, 2024

hemantk-12 Feb 22, 2024

swamirishi Feb 22, 2024

swamirishi Feb 22, 2024

hemantk-12 Feb 22, 2024

hemantk-12 left a comment

swamirishi commented Feb 22, 2024

smengcl commented Feb 22, 2024

HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator #6182

HDDS-10149. New JNI layer for RawSSTFileReader & RawSSTFileReaderIterator #6182

Conversation

swamirishi commented Feb 7, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

swamirishi commented Feb 15, 2024

hemantk-12 left a comment

Choose a reason for hiding this comment

hemantk-12 Feb 21, 2024

Choose a reason for hiding this comment

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

hemantk-12 Feb 22, 2024

Choose a reason for hiding this comment

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

swamirishi Feb 22, 2024

Choose a reason for hiding this comment

hemantk-12 Feb 22, 2024

Choose a reason for hiding this comment

hemantk-12 left a comment

Choose a reason for hiding this comment

swamirishi commented Feb 22, 2024

smengcl commented Feb 22, 2024

swamirishi commented Feb 7, 2024 •

edited

Loading