hemantk-12 commented on code in PR #5165:
URL: https://github.com/apache/ozone/pull/5165#discussion_r1293859502
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -126,11 +135,18 @@ private Optional<UnsignedLong> getNextUnsignedLong()
throws IOException {
}
private void init(ManagedSSTDumpTool sstDumpTool, File sstFile,
- ManagedOptions options)
+ ManagedOptions options, Optional<ManagedSlice> lowerKeyBound,
+ Optional<ManagedSlice> upperKeyBound)
throws NativeLibraryNotLoadedException {
- String[] args = {"--file=" + sstFile.getAbsolutePath(), "--command=scan",
- "--silent"};
- this.sstDumpToolTask = sstDumpTool.run(args, options);
+ Map<String, String> argMap = Maps.newHashMap();
+ argMap.put("file", sstFile.getAbsolutePath());
+ argMap.put("silent", null);
Review Comment:
QQ:
1. What's the default value for this parameter? What happens if you don't
add it to `argMap`?
1. What does it do?
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -89,16 +90,31 @@ public long getEstimatedTotalKeys() throws RocksDBException
{
}
public Stream<String> getKeyStream() throws RocksDBException {
+ return getKeyStream(Optional.empty(), Optional.empty());
+ }
+
+ public Stream<String> getKeyStream(Optional<String> lowerBound,
+ Optional<String> upperBound) throws RocksDBException {
// TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
final MultipleSstFileIterator<String> itr =
new MultipleSstFileIterator<String>(sstFiles) {
private ManagedOptions options;
private ReadOptions readOptions;
+ private Optional<ManagedSlice> lowerBoundSLice;
Review Comment:
nit: It is not general practice to use `Optional` as class parameter (even
IDE shows it as warning). If it is done this way just to avoid null check when
closing them, I would suggest to use `IOUtils#closeQuietly()`.
There are many article about to not use `Optional` as class parameter or
method parameter. Few of them are as:
*
https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments
* http://dolszewski.com/java/java-8-optional-use-cases/
* https://rules.sonarsource.com/java/RSPEC-3553/
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -89,16 +90,31 @@ public long getEstimatedTotalKeys() throws RocksDBException
{
}
public Stream<String> getKeyStream() throws RocksDBException {
+ return getKeyStream(Optional.empty(), Optional.empty());
+ }
+
+ public Stream<String> getKeyStream(Optional<String> lowerBound,
+ Optional<String> upperBound) throws RocksDBException {
// TODO: [SNAPSHOT] Check if default Options and ReadOptions is enough.
final MultipleSstFileIterator<String> itr =
new MultipleSstFileIterator<String>(sstFiles) {
private ManagedOptions options;
private ReadOptions readOptions;
+ private Optional<ManagedSlice> lowerBoundSLice;
+
+ private Optional<ManagedSlice> upperBoundSlice;
Review Comment:
Do you need to close them in
[close](https://github.com/apache/ozone/blob/13da8647d368b5d2a17857d3535d72c772801f3e/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java#L133)?
If yes, please do so otherwise make them local variable to the `init` method.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdb/util/TestManagedSstFileReader.java:
##########
@@ -65,7 +65,7 @@ class TestManagedSstFileReader {
private String createRandomSSTFile(TreeMap<String, Integer> keys)
throws IOException, RocksDBException {
File file = File.createTempFile("tmp_sst_file", ".sst");
- file.deleteOnExit();
+// file.deleteOnExit();
Review Comment:
Is it intentional? If not, please revert it.
##########
hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java:
##########
@@ -464,4 +469,34 @@ public void write(byte[] buf, int off, int len) {
}
}
+ public static String getLexicographicallyLowerString(String val) {
+ char[] charVal = val.toCharArray();
+ charVal[charVal.length - 1] -= 1;
+ return String.valueOf(charVal);
+ }
+
+ public static String getLexicographicallyHigherString(String val) {
+ char[] charVal = val.toCharArray();
+ charVal[charVal.length - 1] += 1;
+ return String.valueOf(charVal);
+ }
+
+ public static List<Optional<String>> getTestingBounds(
Review Comment:
nit: Though I see it is used in `hdds-rocks-native` and
`rocksdb-checkpoint-differ`, I doubt it will be used anywhere else to qualify
to be added in `GenericTestUtils`. I will suggest to add this as util in
`hdds-rocks-native` and reuse in `rocksdb-checkpoint-differ`.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -125,21 +141,34 @@ public void close() throws UncheckedIOException {
public Stream<String> getKeyStreamWithTombstone(
Review Comment:
This is not used anymore. If so, please remove it.
##########
hadoop-hdds/rocks-native/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedSSTDumpIterator.java:
##########
@@ -58,9 +60,16 @@ public abstract class ManagedSSTDumpIterator<T> implements
ClosableIterator<T> {
private AtomicBoolean open;
private StackTraceElement[] stackTrace;
+ public ManagedSSTDumpIterator(ManagedSSTDumpTool sstDumpTool,
Review Comment:
This alignment is not correct as per Ozone coding guideline here and as well
as many other places changed in this PR.
It would be great if you can configure your IDE to
[Ozone-style](https://github.com/apache/ozone/blob/master/hadoop-ozone/dev-support/intellij/ozone-style.xml)
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/ManagedSstFileReader.java:
##########
@@ -89,16 +90,31 @@ public long getEstimatedTotalKeys() throws RocksDBException
{
}
public Stream<String> getKeyStream() throws RocksDBException {
Review Comment:
This is not used anymore. If so, please remove it.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdb/util/TestManagedSstFileReader.java:
##########
@@ -91,19 +91,24 @@ private Map<String, Integer> createKeys(int startRange, int
endRange) {
i -> i % 2));
}
- private Pair<Map<String, Integer>, List<String>> createDummyData(
+ private Pair<TreeMap<String, Integer>, List<String>> createDummyData(
Review Comment:
What's the reason behind changing the type here? It opposes to interface
over implementation.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdb/util/TestManagedSstFileReader.java:
##########
@@ -58,7 +62,7 @@ class TestManagedSstFileReader {
// Key prefix containing all characters, to check if all characters can be
// written & read from rocksdb through SSTDumptool
- private static final String KEY_PREFIX = IntStream.range(0, 256).boxed()
+ private static final String KEY_PREFIX = IntStream.range(1, 256).boxed()
Review Comment:
+1 on adding comment for 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
