[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4722: HDDS-8497. Add leading zeroes on to report table to optimize get Pagecall of Snapdiff

["hemantk-12 (via GitHub)" <gi...@apache.org>]

hemantk-12 commented on code in PR #4722:
URL: https://github.com/apache/ozone/pull/4722#discussion_r1235871093


##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestRocksDbPersistentMap.java:
##########
@@ -131,4 +147,93 @@ public void testRocksDBPersistentMap() throws IOException, RocksDBException {
       }
     }
   }
+
+  /**
+   * Test cases for testRocksDBPersistentMapIterator.
+   */
+  private static Stream<Arguments> testRocksDBPersistentMapIteratorCases() {

Review Comment:
   It is not test, more of testcases. Please update the comment as well.
   ```suggestion
     private static Stream<Arguments> rocksDbPersistentMapIteratorTestCases() {
   ```



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/RocksDbPersistentMap.java:
##########
@@ -133,6 +164,15 @@ public V setValue(V value) {
       @Override
       public void close() {
         iterator.close();
+        if (readOptions != null) {
+          readOptions.close();
+        }
+        if (upperBound.isPresent()) {

Review Comment:
   nit: I know it is true today that if `upperBound` is present  `upperBoundSlice` will be not-null. But it can change in future. It would be better if you have null check on `upperBoundSlice` instead `upperBound.isPresent()`. Same for `lowerBoundSlice`.
   
   ```suggestion
           if (upperBoundSlice != null) {
   ```
   



##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestRocksDbPersistentMap.java:
##########
@@ -131,4 +147,93 @@ public void testRocksDBPersistentMap() throws IOException, RocksDBException {
       }
     }
   }
+
+  /**
+   * Test cases for testRocksDBPersistentMapIterator.
+   */
+  private static Stream<Arguments> testRocksDBPersistentMapIteratorCases() {
+    return Stream.of(
+        Arguments.of(
+            Optional.empty(),
+            Optional.of("key202"),
+            Stream.concat(IntStream.range(0, 100).boxed(),
+                    IntStream.range(200, 300).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList()),
+            Stream.concat(IntStream.range(0, 100).boxed(),
+                    IntStream.range(200, 202).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList())),
+        Arguments.of(Optional.of("key050"),
+            Optional.empty(), Stream.concat(IntStream.range(50, 100).boxed(),
+                    IntStream.range(200, 300).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList()),
+            Stream.concat(IntStream.range(50, 100).boxed(),
+                    IntStream.range(200, 300).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList())
+            ),
+        Arguments.of(Optional.of("key050"),
+            Optional.of("key210"),
+            Stream.concat(IntStream.range(50, 100).boxed(),
+                    IntStream.range(200, 300).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList()),
+            Stream.concat(IntStream.range(50, 100).boxed(),
+                    IntStream.range(200, 210).boxed())
+                .map(i -> Pair.of(String.format("key%03d", i),
+                    String.format("value%03d", i)))
+                .collect(Collectors.toList())
+        ));
+  }
+
+  @ParameterizedTest
+  @MethodSource("testRocksDBPersistentMapIteratorCases")
+  public void testRocksDBPersistentMapIterator(Optional<String> lowerBound,
+              Optional<String> upperBound, List<Pair<String, String>> keys,
+              List<Pair<String, String>> expectedKeys)
+      throws IOException, RocksDBException {
+    ColumnFamilyHandle columnFamily = null;
+    try {
+      final CodecRegistry codecRegistry = CodecRegistry.newBuilder().build();
+      columnFamily = db.get().createColumnFamily(new ColumnFamilyDescriptor(
+          codecRegistry.asRawData("testMap" + id.incrementAndGet()),
+          columnFamilyOptions));
+
+      PersistentMap<String, String> persistentMap = new RocksDbPersistentMap<>(
+          db,
+          columnFamily,
+          codecRegistry,
+          String.class,
+          String.class
+      );
+
+      for (int i = 0; i < keys.size(); i++) {
+        String key = keys.get(i).getKey();
+        String value = keys.get(i).getValue();
+        persistentMap.put(key, value);
+      }
+      ClosableIterator<Map.Entry<String, String>> iterator =
+          persistentMap.iterator(lowerBound, upperBound);
+      int idx = 0;
+      while (iterator.hasNext()) {
+        Map.Entry<String, String> e = iterator.next();
+        Assertions.assertEquals(Pair.of(e.getKey(), e.getValue()),
+            expectedKeys.get(idx));
+        idx += 1;
+      }
+
+    } finally {
+      if (columnFamily != null) {
+        db.get().dropColumnFamily(columnFamily);

Review Comment:
   nit: is it possible the `db` is null?



-- 
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