You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2023/03/10 18:20:41 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
swamirishi opened a new pull request, #4376:
URL: https://github.com/apache/ozone/pull/4376
## 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
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157609407
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -217,37 +270,91 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
Review Comment:
Will do this as part of the refactoring PR.
--
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] smengcl commented on pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1513746979
> @smengcl Can this be merged?
CI passed. nice. Will merge this.
--
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] smengcl commented on a diff in pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1148191737
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
+ CloseableIterator<T> itr) {
+ final Spliterator<T> spliterator = Spliterators
+ .spliteratorUnknownSize(itr, 0);
+ return StreamSupport.stream(spliterator, false).onClose(() -> {
+ try {
+ itr.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ }
+
+ public Stream<String> getKeyStream() throws RocksDBException,
Review Comment:
@swamirishi Pls import `./hadoop-ozone/dev-support/intellij/ozone-style.xml` and use it in your IntelliJ IDE.
https://www.jetbrains.com/help/idea/configuring-code-style.html#import-code-style
--
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] smengcl commented on a diff in pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139306185
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
Review Comment:
nit
```suggestion
// TODO: [SNAPSHOT] Check if default Options and ReadOptions 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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1152464161
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
+ CloseableIterator<T> itr) {
+ final Spliterator<T> spliterator = Spliterators
+ .spliteratorUnknownSize(itr, 0);
+ return StreamSupport.stream(spliterator, false).onClose(() -> {
+ try {
+ itr.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ }
+
+ public Stream<String> getKeyStream() throws RocksDBException,
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144224646
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -597,6 +597,21 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE =
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] adoroszlai commented on pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1464354845
This is based on top of commits that belong to another open PR, so this should be marked as draft:
1. to avoid someone accidentally merging this one before the other
2. to save CI resources, since this one will need to be updated after the other PR gets merged
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1143521203
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
+ * @param value
+ * @return
+ */
+ protected abstract T getTransformedValue(KeyValue value);
+
/**
* Returns the next record from SSTDumpTool.
* @return next Key
* Throws Runtime Exception incase of failure.
Review Comment:
the function checkSanityOfProcess() checks if the exit code is not 0, in that case it throws a RuntimeException.
--
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] smengcl merged pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #4376:
URL: https://github.com/apache/ozone/pull/4376
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1152537956
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CloseableIterator.java:
##########
@@ -0,0 +1,13 @@
+package org.apache.hadoop.hdds.utils;
+
+import java.io.Closeable;
+import java.util.Iterator;
+
+/**
+ * Interface for Implementing CloseableIterators.
+ *
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+public interface CloseableIterator<T> extends Iterator<T>, Closeable {
Review Comment:
Please use [ClosableIterator](https://github.com/apache/ozone/blob/master/hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java#L25).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -170,6 +183,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// TODO: [SNAPSHOT] Load jobs only if it is leader node.
// It could a event-triggered form OM when node is leader and up.
this.loadJobsOnStartUp();
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
Review Comment:
This is not needed.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -170,6 +183,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// TODO: [SNAPSHOT] Load jobs only if it is leader node.
// It could a event-triggered form OM when node is leader and up.
this.loadJobsOnStartUp();
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
Review Comment:
I think it would be better to move this try-catch to `initSSTDumpTool`.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
+ CloseableIterator<T> itr) {
+ final Spliterator<T> spliterator = Spliterators
+ .spliteratorUnknownSize(itr, 0);
+ return StreamSupport.stream(spliterator, false).onClose(() -> {
+ try {
+ itr.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ }
+
+ public Stream<String> getKeyStream() throws RocksDBException,
Review Comment:
It is still not fixed. New code is not indented properly.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139462258
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -92,6 +107,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// Need for Diff Report
this.codecRegistry.addCodec(DiffReportEntry.class,
new OmDBDiffReportEntryCodec());
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
+ } catch (NativeLibraryNotLoadedException e) {
+ LOG.error("Unable to load SSTDumpTool ", e);
+ isNativeRocksToolsLoaded = false;
+ }
+ }
+ }
+
+ private void initSSTDumpTool(OzoneConfiguration conf)
+ throws NativeLibraryNotLoadedException {
+ int threadPoolSize = conf.getInt(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT);
+ int bufferSize = (int)conf.getStorageSize(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE_DEFAULT,
+ StorageUnit.BYTES);
+ ExecutorService executorService = new ThreadPoolExecutor(0,
+ threadPoolSize, 60, TimeUnit.SECONDS,
+ new SynchronousQueue<>(), new ThreadFactoryBuilder()
Review Comment:
FYI: [SynchronousQueue](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/SynchronousQueue.html) is unbounded queue and can cause memory pressure.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157622929
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/CloseableIterator.java:
##########
@@ -0,0 +1,13 @@
+package org.apache.hadoop.hdds.utils;
+
+import java.io.Closeable;
+import java.util.Iterator;
+
+/**
+ * Interface for Implementing CloseableIterators.
+ *
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+public interface CloseableIterator<T> extends Iterator<T>, Closeable {
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1143547880
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
Review Comment:
it gets sst files corresponding to the column family.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1145346988
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
+ throw new IllegalStateException(ex);
+ }
+ }
if (bucketLayout.isFileSystemOptimized()) {
+ // add to object ID map for directory.
final Table<String, OmDirectoryInfo> fsDirTable =
fromSnapshot.getMetadataManager().getDirectoryTable();
final Table<String, OmDirectoryInfo> tsDirTable =
toSnapshot.getMetadataManager().getDirectoryTable();
+ tablesToLookUp = Collections.singletonList(fsDirTable.getName());
final Set<String> deltaFilesForDirTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsDirTable.getName()), fsInfo, tsInfo,
- useFullDiff, tablePrefixes);
- addToObjectIdMap(fsDirTable,
- tsDirTable,
- deltaFilesForDirTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ getDeltaFiles(fromSnapshot, toSnapshot, tablesToLookUp, fsInfo,
+ tsInfo, useFullDiff, tablePrefixes);
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ try {
+ // Workaround to handle deletes if use of native rockstools for
+ // reading tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(false, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
+ throw new IllegalStateException(ex);
+ }
+ }
Review Comment:
nit: This (line 427-458) is duplicate of lines 384-415. You may move this to a function and call it from both the places.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
+ CloseableIterator<T> itr) {
+ final Spliterator<T> spliterator = Spliterators
+ .spliteratorUnknownSize(itr, 0);
+ return StreamSupport.stream(spliterator, false).onClose(() -> {
+ try {
+ itr.close();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ });
+ }
+
+ public Stream<String> getKeyStream() throws RocksDBException,
Review Comment:
nit: line break in your code is indented by 8 spaces instead of 4. Please configure your IDE accordingly and indent the code that way.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -366,22 +484,26 @@ private void generateSnapshotDiffReport(final String jobId,
private void addToObjectIdMap(Table<String, ? extends WithObjectID> fsTable,
Table<String, ? extends WithObjectID> tsTable,
- Set<String> deltaFiles,
+ Pair<Boolean, Set<String>>
+ isNativeRocksToolsLoadedDeltaFilesPair,
PersistentMap<byte[], byte[]> oldObjIdToKeyMap,
PersistentMap<byte[], byte[]> newObjIdToKeyMap,
PersistentSet<byte[]> objectIDsToCheck,
Map<String, String> tablePrefixes)
- throws IOException {
+ throws IOException, NativeLibraryNotLoadedException {
+ Set<String> deltaFiles = isNativeRocksToolsLoadedDeltaFilesPair.getRight();
+ boolean nativeRocksToolsLoaded =
+ isNativeRocksToolsLoadedDeltaFilesPair.getLeft();
+ boolean isDirectoryTable =
+ fsTable.getName().equals(OmMetadataManagerImpl.DIRECTORY_TABLE);
Review Comment:
nit: this can be move after line no 502.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
Review Comment:
```suggestion
// This code should be never executed.
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
Review Comment:
1. 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.
2. `ManagedReadOptions` to declare variable as well.
```
ManagedReadOptions readOptions = new ManagedReadOptions();
```
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
+ * @param value
+ * @return
+ */
+ protected abstract T getTransformedValue(KeyValue value);
+
/**
* Returns the next record from SSTDumpTool.
* @return next Key
* Throws Runtime Exception incase of failure.
Review Comment:
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.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
Review Comment:
```suggestion
// TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read
// tombstone.
```
--
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] smengcl commented on a diff in pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144100586
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
Review Comment:
oh nvm `default` is just a CF that has to be added but not actually used by OM.
--
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 #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139404653
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +802,12 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
+ /**
+ * Interface for Implementing CloseableIterators.
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+ public interface CloseableIterator<T> extends Iterator<T>, Closeable {
Review Comment:
I think it is unnecessary interface. You can directly implement `Iterator` and `Closeable` wherever is needed.
If you think it is really needed, it doesn't have to be in Util class.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157833009
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
Review Comment:
Sorry to add this again but indentation is still not fixed. I believe your IDE is not using [Ozone code style](https://www.jetbrains.com/help/idea/configuring-code-style.html#import-code-style).
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
I think It was OK to override `close()` method and leave exception as RunTimeException. Also any particular reason for changing it from `AutoCloseable` to `Closeable`.
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -25,18 +26,17 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
-import java.util.Iterator;
+import java.util.NoSuchElementException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
/**
* Iterator to Parse output of RocksDBSSTDumpTool.
*/
-public class ManagedSSTDumpIterator implements
- Iterator<ManagedSSTDumpIterator.KeyValue>, AutoCloseable {
- private static final String SST_DUMP_TOOL_CLASS =
- "org.apache.hadoop.hdds.utils.db.managed.ManagedSSTDumpTool";
+public abstract class ManagedSSTDumpIterator<T> implements
Review Comment:
Suggestion: We can override `finalize` similar to other `ManagedClass` to log warning if object doesn't get closed.
```
@Override
protected void finalize() throws Throwable {
ManagedRocksObjectUtils.assertClosed(this);
super.finalize();
}
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +800,5 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
Review Comment:
This is not needed.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1160146420
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
AutoClosable throws any exception, since we are throwing IOException better to use Closeable, moreover the previous implementation was not throwing any exceptions.
--
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] smengcl commented on pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1508893567
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
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1165891319
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +800,5 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
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 pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1483230274
> 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.
--
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] smengcl commented on pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1470817999
@hemantk-12 Would you take a look at this?
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157606486
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -607,6 +607,20 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
Review Comment:
done
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -170,6 +183,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// TODO: [SNAPSHOT] Load jobs only if it is leader node.
// It could a event-triggered form OM when node is leader and up.
this.loadJobsOnStartUp();
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157609407
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -217,37 +270,91 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
Review Comment:
Will do this as part of the refactoring changes & optimization changes [HDDS-8376](https://issues.apache.org/jira/browse/HDDS-8376)
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157621433
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -170,6 +183,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// TODO: [SNAPSHOT] Load jobs only if it is leader node.
// It could a event-triggered form OM when node is leader and up.
this.loadJobsOnStartUp();
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = 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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144226822
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144224307
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
+ * @param value
+ * @return
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1166005520
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,80 +52,176 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public static <T> Stream<T> getStreamFromIterator(
Review Comment:
done
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1157622123
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
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] smengcl commented on pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1496738766
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
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1160292498
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
1. If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception from [Oracle documentation]( https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html). I don't think client can do anything in this scenario.
2. `It is better to avoid runtime exceptions as much as possible.` is not true. If you look for exception in any other new language like Kotlin, they don't even have checked exception.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1143527382
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
Review Comment:
We are closing the options & readoptions in the close function.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144223766
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -597,6 +597,21 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE =
+ "ozone.om.snapshot.sst_dumptool.pool.size";
+
+ public static final int
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT = 1;
+
+ public static final String
Review Comment:
done
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -139,8 +146,9 @@ public KeyValue next() {
if (currentKey != null) {
currentKey.setValue(stdoutString.substring(0,
Math.max(stdoutString.length() - 1, 0)));
+ return getTransformedValue(currentKey);
}
- return currentKey;
+ throw new NoSuchElementException("No more records found");
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] smengcl commented on a diff in pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139292590
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
Review Comment:
`ozone.om.snapshot.sst_dumptool.buffer.size` should be added here as well.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1485868695
@hemantk-12 @swamirishi Can we mark all the comments that are resolved 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] smengcl commented on a diff in pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139358505
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
Review Comment:
```suggestion
}
public Stream<String> getKeyStreamWithTombstone(
```
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144225928
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144225105
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +802,12 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
+ /**
+ * Interface for Implementing CloseableIterators.
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+ public interface CloseableIterator<T> extends Iterator<T>, Closeable {
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1143579179
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -131,6 +177,13 @@ private DifferSnapshotInfo getDSIFromSI(SnapshotInfo snapshotInfo,
getTablePrefixes(snapshotOMMM, volumeName, bucketName));
}
+ private Set<String> getSSTFileListForSnapshot(OmSnapshot snapshot,
+ List<String> tablesToLookUp) throws RocksDBException {
+ return RdbUtil.getSSTFilesForComparison(snapshot
+ .getMetadataManager().getStore().getDbLocation()
+ .getPath(), tablesToLookUp);
+ }
+
@SuppressWarnings("checkstyle:methodlength")
public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Review Comment:
This could be a separate PR. [HDDS-7798](https://github.com/apache/ozone/pull/4187)
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1160147187
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
It is better to avoid runtime exceptions as much as possible.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1160292498
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
1. If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception from [Oracle documentation]( https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html). I don't think client can do anything in this scenario.
2. `It is better to avoid runtime exceptions as much as possible.` is not true. If you look for exception in any other new language like Kotlin, they don't even have checked exception. Or any other cloud API like AWS.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1160292498
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/util/ClosableIterator.java:
##########
@@ -17,12 +17,11 @@
*/
package org.apache.hadoop.util;
+import java.io.Closeable;
import java.util.Iterator;
/**
* An {@link Iterator} that may hold resources until it is closed.
*/
-public interface ClosableIterator<E> extends Iterator<E>, AutoCloseable {
- @Override
- void close();
+public interface ClosableIterator<E> extends Iterator<E>, Closeable {
Review Comment:
1. If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception from [Oracle documentation]( https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html). I don't think client can do anything in this scenario.
2. `It is better to avoid runtime exceptions as much as possible.` is not true. If you look for exception in any other new language like Kotlin, they don't even have checked exception.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1152463285
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -366,22 +484,26 @@ private void generateSnapshotDiffReport(final String jobId,
private void addToObjectIdMap(Table<String, ? extends WithObjectID> fsTable,
Table<String, ? extends WithObjectID> tsTable,
- Set<String> deltaFiles,
+ Pair<Boolean, Set<String>>
+ isNativeRocksToolsLoadedDeltaFilesPair,
PersistentMap<byte[], byte[]> oldObjIdToKeyMap,
PersistentMap<byte[], byte[]> newObjIdToKeyMap,
PersistentSet<byte[]> objectIDsToCheck,
Map<String, String> tablePrefixes)
- throws IOException {
+ throws IOException, NativeLibraryNotLoadedException {
+ Set<String> deltaFiles = isNativeRocksToolsLoadedDeltaFilesPair.getRight();
+ boolean nativeRocksToolsLoaded =
+ isNativeRocksToolsLoadedDeltaFilesPair.getLeft();
+ boolean isDirectoryTable =
+ fsTable.getName().equals(OmMetadataManagerImpl.DIRECTORY_TABLE);
Review Comment:
done
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
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 #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139407239
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -597,6 +597,21 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE =
Review Comment:
nit: just to be in alignment with other configs.
```suggestion
OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE =
"ozone.om.snapshot.sst.dumptool.pool.size";
```
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
+ * @param value
+ * @return
Review Comment:
Either add the return type or remove it.
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
+ * @param value
+ * @return
+ */
+ protected abstract T getTransformedValue(KeyValue value);
+
/**
* Returns the next record from SSTDumpTool.
* @return next Key
* Throws Runtime Exception incase of failure.
Review Comment:
Should this need to be changed to `NoSuchElementException`?
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -89,37 +179,33 @@ public boolean hasNext() {
}
@Override
- public String next() {
+ public T next() {
if (hasNext()) {
- final String value = new String(currentFileIterator.key(), UTF_8);
- currentFileIterator.next();
- return value;
+ return currentFileIterator.next();
+// final String value = new String(currentFileIterator.key(), UTF_8);
Review Comment:
nit: Remove this if not needed.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -217,37 +270,91 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should never be never executed.
Review Comment:
Comment is not redundant. You could use `IllegalStateException` instead of `RuntimeException` to denote the this situation should not happen.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +802,12 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
+ /**
+ * Interface for Implementing CloseableIterators.
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+ public interface CloseableIterator<T> extends Iterator<T>, Closeable {
Review Comment:
I think it is unnecessary interface. You can directly implement `Iterator` and `Closeable` wherever is needed.
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
Review Comment:
```suggestion
* Transforms key to a certain value.
```
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -139,8 +146,9 @@ public KeyValue next() {
if (currentKey != null) {
currentKey.setValue(stdoutString.substring(0,
Math.max(stdoutString.length() - 1, 0)));
+ return getTransformedValue(currentKey);
}
- return currentKey;
+ throw new NoSuchElementException("No more records found");
Review Comment:
```suggestion
throw new NoSuchElementException("No more elements found.");
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
Review Comment:
```suggestion
final MultipleSstFileIterator<String> itr =
new MultipleSstFileIterator<String>(sstFiles) {
ManagedOptions options = new ManagedOptions();
ManagedReadOptions readOptions = new ManagedReadOptions();
@Override
protected HddsUtils.CloseableIterator<String>
initNewKeyIteratorForFile(String file) throws RocksDBException {
return new ManagedSstFileIterator<String>(file, options,
readOptions) {
@Override
protected String getIteratorValue(
SstFileReaderIterator iterator) {
return new String(iterator.key(), UTF_8);
}
};
}
@Override
public void close() throws IOException {
super.close();
options.close();
readOptions.close();
}
};
return RdbUtil.getStreamFromIterator(itr);
```
1. Defining `options` and `readOptions` inside implementation of MultipleSstFileIterator would be better. Otherwise if something gets added in between and don't close it properly, there will be a memory leak.
1. Use `ManagedReadOptions` instead of `ReadOptions`.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
+ ManagedOptions options = new ManagedOptions();
Review Comment:
Move this inside implementation (after line 85).
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
+ ManagedOptions options = new ManagedOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file)
+ throws NativeLibraryNotLoadedException, IOException {
+ return new ManagedSSTDumpIterator<String>(sstDumpTool, file,
+ options) {
+ @Override
+ protected String getTransformedValue(KeyValue value) {
+ return value.getKey();
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+
+ private abstract static class ManagedSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
+ private SstFileReader fileReader;
+ private SstFileReaderIterator fileReaderIterator;
+
+ ManagedSstFileIterator(String path, ManagedOptions options,
+ ReadOptions readOptions)
+ throws RocksDBException {
+ this.fileReader = new SstFileReader(options);
+ this.fileReader.open(path);
+ this.fileReaderIterator = fileReader.newIterator(readOptions);
+ fileReaderIterator.seekToFirst();
+ }
+
+ @Override
+ public void close() throws IOException {
+ this.fileReaderIterator.close();
+ this.fileReader.close();
+ }
+
+ @Override
+ public boolean hasNext() {
+ return fileReaderIterator.isValid();
+ }
+
+ protected abstract T getIteratorValue(SstFileReaderIterator iterator);
+
+ @Override
+ public T next() {
+ T value = getIteratorValue(fileReaderIterator);
+ fileReaderIterator.next();
+ return value;
+ }
}
- private static final class ManagedSstFileIterator implements
- Iterator<String>, Closeable {
+ private abstract static class MultipleSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
private final Iterator<String> fileNameIterator;
- private final Options options;
- private final ReadOptions readOptions;
+
private String currentFile;
private SstFileReader currentFileReader;
- private SstFileReaderIterator currentFileIterator;
+ private HddsUtils.CloseableIterator<T> currentFileIterator;
- private ManagedSstFileIterator(Collection<String> files)
- throws RocksDBException {
- // TODO: Check if default Options and ReadOptions is enough.
- this.options = new Options();
- this.readOptions = new ReadOptions();
+ private MultipleSstFileIterator(Collection<String> files)
+ throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
this.fileNameIterator = files.iterator();
moveToNextFile();
}
+ protected abstract HddsUtils.CloseableIterator<T> initNewKeyIteratorForFile(
+ String file) throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException;
+
@Override
public boolean hasNext() {
try {
do {
- if (currentFileIterator.isValid()) {
+ if (currentFileIterator.hasNext()) {
return true;
}
} while (moveToNextFile());
- } catch (RocksDBException e) {
+ } catch (IOException | RocksDBException |
+ NativeLibraryNotLoadedException e) {
// TODO: This exception has to be handled by the caller.
Review Comment:
```suggestion
// TODO: [Snapshot] This exception has to be handled by the caller.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -217,37 +270,91 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should never be never executed.
+ throw new RuntimeException(ex);
+ }
+ }
if (bucketLayout.isFileSystemOptimized()) {
// add to object ID map for directory.
final Table<String, OmDirectoryInfo> fsDirTable =
fromSnapshot.getMetadataManager().getDirectoryTable();
final Table<String, OmDirectoryInfo> tsDirTable =
toSnapshot.getMetadataManager().getDirectoryTable();
+ tablesToLookUp = Collections.singletonList(fsDirTable.getName());
final Set<String> deltaFilesForDirTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsDirTable.getName()), fsInfo, tsInfo,
- useFullDiff, tablePrefixes);
- addToObjectIdMap(fsDirTable,
- tsDirTable,
- deltaFilesForDirTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ getDeltaFiles(fromSnapshot, toSnapshot, tablesToLookUp, fsInfo,
+ tsInfo, useFullDiff, tablePrefixes);
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ try {
+ // Workaround to handle deletes if use of native rockstools for
+ // reading tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(false, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should never be never executed.
Review Comment:
Same as above. Use `IllegalStateException`.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
+ ManagedOptions options = new ManagedOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file)
+ throws NativeLibraryNotLoadedException, IOException {
+ return new ManagedSSTDumpIterator<String>(sstDumpTool, file,
+ options) {
+ @Override
+ protected String getTransformedValue(KeyValue value) {
+ return value.getKey();
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+
+ private abstract static class ManagedSstFileIterator<T> implements
Review Comment:
`ManagedSstFileIterator` and `MultipleSstFileIterator` don't need to be generic. Both of them are internal to `ManagedSstFileReader` and `String` type. Making it generic is adding readability complexity.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -597,6 +597,21 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE =
+ "ozone.om.snapshot.sst_dumptool.pool.size";
+
+ public static final int
+ OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT = 1;
+
+ public static final String
Review Comment:
```suggestion
public static final String
OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE =
"ozone.om.snapshot.sst.dumptool.buffer.size";
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
@@ -58,4 +65,17 @@ public static Set<String> getSSTFilesForComparison(final String dbLocation,
}
}
+ public static <T> Stream<T> getStreamFromIterator(
Review Comment:
I don't think it has to be a Util as of now and can be internal to `ManagedSstFileReader`.
Same goes for `getSSTFilesForComparison` but you can leave that as it is for now.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -92,6 +107,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// Need for Diff Report
this.codecRegistry.addCodec(DiffReportEntry.class,
new OmDBDiffReportEntryCodec());
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
+ } catch (NativeLibraryNotLoadedException e) {
+ LOG.error("Unable to load SSTDumpTool ", e);
+ isNativeRocksToolsLoaded = false;
+ }
+ }
+ }
+
+ private void initSSTDumpTool(OzoneConfiguration conf)
+ throws NativeLibraryNotLoadedException {
+ int threadPoolSize = conf.getInt(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT);
+ int bufferSize = (int)conf.getStorageSize(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE_DEFAULT,
+ StorageUnit.BYTES);
+ ExecutorService executorService = new ThreadPoolExecutor(0,
+ threadPoolSize, 60, TimeUnit.SECONDS,
+ new SynchronousQueue<>(), new ThreadFactoryBuilder()
Review Comment:
FYI: [SynchronousQueue](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/SynchronousQueue.html) is unbounded key and can cause memory pressure.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -89,37 +179,33 @@ public boolean hasNext() {
}
@Override
- public String next() {
+ public T next() {
if (hasNext()) {
- final String value = new String(currentFileIterator.key(), UTF_8);
- currentFileIterator.next();
- return value;
+ return currentFileIterator.next();
+// final String value = new String(currentFileIterator.key(), UTF_8);
}
throw new NoSuchElementException("No more keys");
Review Comment:
nit:
```suggestion
throw new NoSuchElementException("No more elements found.");
```
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1150991044
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -308,36 +371,91 @@ private void generateSnapshotDiffReport(final String jobId,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
+ final Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(
+ fromSnapshot, toSnapshot, tablesToLookUp, fsInfo, tsInfo,
useFullDiff, tablePrefixes);
- addToObjectIdMap(fsKeyTable,
- tsKeyTable,
- deltaFilesForKeyOrFileTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ // Workaround to handle deletes if native rockstools for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ // Workaround to handle deletes if use of native rockstools for reading
+ // tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ try {
+ addToObjectIdMap(fsKeyTable,
+ tsKeyTable,
+ Pair.of(false, deltaFilesForKeyOrFileTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
+ throw new IllegalStateException(ex);
+ }
+ }
if (bucketLayout.isFileSystemOptimized()) {
+ // add to object ID map for directory.
final Table<String, OmDirectoryInfo> fsDirTable =
fromSnapshot.getMetadataManager().getDirectoryTable();
final Table<String, OmDirectoryInfo> tsDirTable =
toSnapshot.getMetadataManager().getDirectoryTable();
+ tablesToLookUp = Collections.singletonList(fsDirTable.getName());
final Set<String> deltaFilesForDirTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsDirTable.getName()), fsInfo, tsInfo,
- useFullDiff, tablePrefixes);
- addToObjectIdMap(fsDirTable,
- tsDirTable,
- deltaFilesForDirTable,
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
+ getDeltaFiles(fromSnapshot, toSnapshot, tablesToLookUp, fsInfo,
+ tsInfo, useFullDiff, tablePrefixes);
+ if (!isNativeRocksToolsLoaded) {
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ }
+ try {
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(isNativeRocksToolsLoaded, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ try {
+ // Workaround to handle deletes if use of native rockstools for
+ // reading tombstone fails.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read to
+ // read tombstone
+ deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
+ fromSnapshot, tablesToLookUp));
+ addToObjectIdMap(fsDirTable,
+ tsDirTable,
+ Pair.of(false, deltaFilesForDirTable),
+ objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot,
+ objectIDsToCheckMap,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException ex) {
+ //This code should be never executed.
+ throw new IllegalStateException(ex);
+ }
+ }
Review Comment:
I believe we are doing redundant checks on the sst files, when bucket is FSOptimized. This would change a lot of things the way code has been implemented. Will take this up in the next PR.
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1165894032
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -25,18 +26,17 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
-import java.util.Iterator;
+import java.util.NoSuchElementException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
/**
* Iterator to Parse output of RocksDBSSTDumpTool.
*/
-public class ManagedSSTDumpIterator implements
- Iterator<ManagedSSTDumpIterator.KeyValue>, AutoCloseable {
- private static final String SST_DUMP_TOOL_CLASS =
- "org.apache.hadoop.hdds.utils.db.managed.ManagedSSTDumpTool";
+public abstract class ManagedSSTDumpIterator<T> implements
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139404653
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -800,4 +802,12 @@ public static Map<String, String> processForLogging(OzoneConfiguration conf) {
}
return sortedOzoneProps;
}
+
+ /**
+ * Interface for Implementing CloseableIterators.
+ * @param <T> Generic Parameter for Iterating values of type 'T'
+ */
+ public interface CloseableIterator<T> extends Iterator<T>, Closeable {
Review Comment:
~I think it is unnecessary interface. You can directly implement `Iterator` and `Closeable` wherever is needed.~
~If you think it is really needed, it doesn't have to be in Util class.~
I see why it has to be this way. But I think it deserves its own class.
--
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] smengcl commented on a diff in pull request #4376: HDDS-8137. Snapdiff to use tombstone entries in SST file
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139358133
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
Review Comment:
nit
```suggestion
// TODO: [SNAPSHOT] Check if default Options 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] smengcl commented on a diff in pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1139828081
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
Review Comment:
Is `getSSTFilesForComparison()` here in this class filtering out SSTs with only default CF on line 59?
Or is it just a dummy CF name here for the SST reader which doesn't care?
It is called from `getSSTFileListForSnapshot()` thus the question.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -131,6 +177,13 @@ private DifferSnapshotInfo getDSIFromSI(SnapshotInfo snapshotInfo,
getTablePrefixes(snapshotOMMM, volumeName, bucketName));
}
+ private Set<String> getSSTFileListForSnapshot(OmSnapshot snapshot,
+ List<String> tablesToLookUp) throws RocksDBException {
+ return RdbUtil.getSSTFilesForComparison(snapshot
+ .getMetadataManager().getStore().getDbLocation()
+ .getPath(), tablesToLookUp);
+ }
+
@SuppressWarnings("checkstyle:methodlength")
public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Review Comment:
Need to have more test cases to cover the branch additions in this method.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
+ ManagedOptions options = new ManagedOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file)
+ throws NativeLibraryNotLoadedException, IOException {
+ return new ManagedSSTDumpIterator<String>(sstDumpTool, file,
+ options) {
+ @Override
+ protected String getTransformedValue(KeyValue value) {
+ return value.getKey();
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+
+ private abstract static class ManagedSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
+ private SstFileReader fileReader;
+ private SstFileReaderIterator fileReaderIterator;
+
+ ManagedSstFileIterator(String path, ManagedOptions options,
+ ReadOptions readOptions)
+ throws RocksDBException {
+ this.fileReader = new SstFileReader(options);
+ this.fileReader.open(path);
+ this.fileReaderIterator = fileReader.newIterator(readOptions);
+ fileReaderIterator.seekToFirst();
+ }
+
+ @Override
+ public void close() throws IOException {
+ this.fileReaderIterator.close();
+ this.fileReader.close();
+ }
+
+ @Override
+ public boolean hasNext() {
+ return fileReaderIterator.isValid();
+ }
+
+ protected abstract T getIteratorValue(SstFileReaderIterator iterator);
+
+ @Override
+ public T next() {
+ T value = getIteratorValue(fileReaderIterator);
+ fileReaderIterator.next();
+ return value;
+ }
}
- private static final class ManagedSstFileIterator implements
- Iterator<String>, Closeable {
+ private abstract static class MultipleSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
private final Iterator<String> fileNameIterator;
- private final Options options;
- private final ReadOptions readOptions;
+
private String currentFile;
private SstFileReader currentFileReader;
- private SstFileReaderIterator currentFileIterator;
+ private HddsUtils.CloseableIterator<T> currentFileIterator;
- private ManagedSstFileIterator(Collection<String> files)
- throws RocksDBException {
- // TODO: Check if default Options and ReadOptions is enough.
- this.options = new Options();
- this.readOptions = new ReadOptions();
+ private MultipleSstFileIterator(Collection<String> files)
+ throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
this.fileNameIterator = files.iterator();
moveToNextFile();
}
+ protected abstract HddsUtils.CloseableIterator<T> initNewKeyIteratorForFile(
Review Comment:
```suggestion
protected abstract HddsUtils.CloseableIterator<T> getKeyIteratorForFile(
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -92,6 +107,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// Need for Diff Report
this.codecRegistry.addCodec(DiffReportEntry.class,
new OmDBDiffReportEntryCodec());
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
+ } catch (NativeLibraryNotLoadedException e) {
+ LOG.error("Unable to load SSTDumpTool ", e);
+ isNativeRocksToolsLoaded = false;
+ }
+ }
+ }
+
+ private void initSSTDumpTool(OzoneConfiguration conf)
+ throws NativeLibraryNotLoadedException {
+ int threadPoolSize = conf.getInt(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT);
+ int bufferSize = (int)conf.getStorageSize(
Review Comment:
```suggestion
int bufferSize = (int) conf.getStorageSize(
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -217,37 +270,91 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
-
- final Set<String> deltaFilesForKeyOrFileTable =
- getDeltaFiles(fromSnapshot, toSnapshot,
- Collections.singletonList(fsKeyTable.getName()), fsInfo, tsInfo,
+ List<String> tablesToLookUp =
+ Collections.singletonList(fsKeyTable.getName());
Review Comment:
Looks like we will never need to explicitly add `fileTable` here since `getKeyTable()` auto picks the right one. In this case, can we just use `String` rather than a `List`?
Write more code comments inline.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -92,6 +107,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// Need for Diff Report
this.codecRegistry.addCodec(DiffReportEntry.class,
new OmDBDiffReportEntryCodec());
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
+ } catch (NativeLibraryNotLoadedException e) {
+ LOG.error("Unable to load SSTDumpTool ", e);
Review Comment:
```suggestion
LOG.error("Unable to load SSTDumpTool", e);
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -46,41 +47,130 @@ public class ManagedSstFileReader {
public ManagedSstFileReader(final Collection<String> sstFiles) {
this.sstFiles = sstFiles;
}
- public Stream<String> getKeyStream() throws RocksDBException {
- final ManagedSstFileIterator itr = new ManagedSstFileIterator(sstFiles);
- final Spliterator<String> spliterator = Spliterators
- .spliteratorUnknownSize(itr, 0);
- return StreamSupport.stream(spliterator, false).onClose(itr::close);
+
+ public Stream<String> getKeyStream() throws RocksDBException,
+ NativeLibraryNotLoadedException, IOException {
+ //TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
+ ManagedOptions options = new ManagedOptions();
+ ReadOptions readOptions = new ReadOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file) throws RocksDBException {
+ return new ManagedSstFileIterator<String>(file, options,
+ readOptions) {
+ @Override
+ protected String getIteratorValue(
+ SstFileReaderIterator iterator) {
+ return new String(iterator.key(), UTF_8);
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ readOptions.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+ public Stream<String> getKeyStreamWithTombstone(
+ ManagedSSTDumpTool sstDumpTool) throws IOException, RocksDBException,
+ NativeLibraryNotLoadedException {
+ //TODO: [SNAPSHOT] Check if default Options is enough.
+ ManagedOptions options = new ManagedOptions();
+ final MultipleSstFileIterator<String> itr =
+ new MultipleSstFileIterator<String>(sstFiles) {
+ @Override
+ protected HddsUtils.CloseableIterator<String>
+ initNewKeyIteratorForFile(String file)
+ throws NativeLibraryNotLoadedException, IOException {
+ return new ManagedSSTDumpIterator<String>(sstDumpTool, file,
+ options) {
+ @Override
+ protected String getTransformedValue(KeyValue value) {
+ return value.getKey();
+ }
+ };
+ }
+
+ @Override
+ public void close() throws IOException {
+ super.close();
+ options.close();
+ }
+ };
+ return RdbUtil.getStreamFromIterator(itr);
+ }
+
+ private abstract static class ManagedSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
+ private SstFileReader fileReader;
+ private SstFileReaderIterator fileReaderIterator;
+
+ ManagedSstFileIterator(String path, ManagedOptions options,
+ ReadOptions readOptions)
+ throws RocksDBException {
+ this.fileReader = new SstFileReader(options);
+ this.fileReader.open(path);
+ this.fileReaderIterator = fileReader.newIterator(readOptions);
+ fileReaderIterator.seekToFirst();
+ }
+
+ @Override
+ public void close() throws IOException {
+ this.fileReaderIterator.close();
+ this.fileReader.close();
+ }
+
+ @Override
+ public boolean hasNext() {
+ return fileReaderIterator.isValid();
+ }
+
+ protected abstract T getIteratorValue(SstFileReaderIterator iterator);
+
+ @Override
+ public T next() {
+ T value = getIteratorValue(fileReaderIterator);
+ fileReaderIterator.next();
+ return value;
+ }
}
- private static final class ManagedSstFileIterator implements
- Iterator<String>, Closeable {
+ private abstract static class MultipleSstFileIterator<T> implements
+ HddsUtils.CloseableIterator<T> {
private final Iterator<String> fileNameIterator;
- private final Options options;
- private final ReadOptions readOptions;
+
private String currentFile;
private SstFileReader currentFileReader;
Review Comment:
This doesn't seem to be used anywhere.
```suggestion
```
--
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 pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1513287025
@smengcl Can this be merged?
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1154972139
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java:
##########
@@ -607,6 +607,20 @@ public final class OzoneConfigKeys {
OZONE_OM_SNAPSHOT_COMPACTION_DAG_PRUNE_DAEMON_RUN_INTERVAL =
"ozone.om.snapshot.compaction.dag.prune.daemon.run.interval";
+ public static final String
Review Comment:
Nit: these config keys and values are related to OM and should be added to [OMConfigKeys](https://github.com/apache/ozone/blob/184bfc21189583f857280933270f134dcac68879/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java#L30).
Previously OM related configs added here should be moved to OMConfigKeys as well but not as part of this PR. Can be done as part of [HDDS-8322](https://issues.apache.org/jira/browse/HDDS-8322).
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1143576734
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -92,6 +107,37 @@ public SnapshotDiffManager(ManagedRocksDB db,
// Need for Diff Report
this.codecRegistry.addCodec(DiffReportEntry.class,
new OmDBDiffReportEntryCodec());
+ isNativeRocksToolsLoaded = NativeLibraryLoader.getInstance()
+ .loadLibrary(NativeConstants.ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (isNativeRocksToolsLoaded) {
+ try {
+ initSSTDumpTool(configuration);
+ isNativeRocksToolsLoaded = true;
+ } catch (NativeLibraryNotLoadedException e) {
+ LOG.error("Unable to load SSTDumpTool ", e);
+ isNativeRocksToolsLoaded = false;
+ }
+ }
+ }
+
+ private void initSSTDumpTool(OzoneConfiguration conf)
+ throws NativeLibraryNotLoadedException {
+ int threadPoolSize = conf.getInt(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_POOL_SIZE_DEFAULT);
+ int bufferSize = (int)conf.getStorageSize(
+ OzoneConfigKeys.OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE,
+ OzoneConfigKeys
+ .OZONE_OM_SNAPSHOT_SST_DUMPTOOL_EXECUTOR_BUFFER_SIZE_DEFAULT,
+ StorageUnit.BYTES);
+ ExecutorService executorService = new ThreadPoolExecutor(0,
+ threadPoolSize, 60, TimeUnit.SECONDS,
+ new SynchronousQueue<>(), new ThreadFactoryBuilder()
Review Comment:
the queueSize would be equal to the number of snapdiff operations running. So it shouldn't be a problem if we are going to cap the number of snapdiff operations.
--
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] smengcl commented on pull request #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4376:
URL: https://github.com/apache/ozone/pull/4376#issuecomment-1478743206
@swamirishi Can you resolve the comments that are addressed?
--
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144223983
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -116,13 +116,20 @@ public boolean hasNext() {
return nextKey != null;
}
+ /**
+ * Transform function to transform key to a certain value.
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 #4376: HDDS-8137. [Snapshot] SnapDiff to use tombstone entries in SST files
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4376:
URL: https://github.com/apache/ozone/pull/4376#discussion_r1144226938
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
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