hemantk-12 commented on code in PR #4722:
URL: https://github.com/apache/ozone/pull/4722#discussion_r1198370853
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/PersistentMap.java:
##########
@@ -32,5 +34,10 @@
void remove(K key);
- ClosableIterator<Map.Entry<K, V>> iterator();
+ default ClosableIterator<Map.Entry<K, V>> iterator() {
+ return this.iterator(Optional.empty(), Optional.empty());
+ }
+
+ ClosableIterator<Map.Entry<K, V>> iterator(Optional<K> lowerBoundKey,
Review Comment:
Would it be better if we provide iterator with prefix instead of lower and
upper bound similar to `RDBStoreIterator`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/RocksDbPersistentMap.java:
##########
@@ -85,9 +90,34 @@ public void remove(K key) {
}
@Override
- public ClosableIterator<Map.Entry<K, V>> iterator() {
- ManagedRocksIterator iterator =
- new ManagedRocksIterator(db.get().newIterator(columnFamilyHandle));
+ public ClosableIterator<Map.Entry<K, V>> iterator(Optional<K> lowerBound,
+ Optional<K> upperBound) {
+ final ManagedReadOptions readOptions;
+ RocksIterator rocksIterator;
+ if (lowerBound.isPresent() || upperBound.isPresent()) {
+ readOptions = new ManagedReadOptions();
+ try {
+ if (lowerBound.isPresent()) {
+ readOptions.setIterateLowerBound(new ManagedSlice(
+ codecRegistry.asRawData(lowerBound.get())));
+ }
+
+ if (upperBound.isPresent()) {
+ readOptions.setIterateLowerBound(new ManagedSlice(
Review Comment:
``` suggestion
readOptions.setIterateUpperBound(new ManagedSlice(
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -444,26 +455,34 @@ private SnapshotDiffReportOzone createPageResponse(
boolean hasMoreEntries = true;
- int idx;
- for (idx = index; idx - index < pageSize; idx++) {
- byte[] rawKey =
- codecRegistry.asRawData(snapDiffJob.getJobId() + DELIMITER + idx);
- byte[] bytes = snapDiffReportTable.get(rawKey);
- if (bytes == null) {
+ byte[] lowerIndex = codecRegistry.asRawData(getReportKeyForIndex(
+ snapDiffJob.getJobId(), index));
+ byte[] upperIndex = codecRegistry.asRawData(getReportKeyForIndex(
+ snapDiffJob.getJobId(), index + pageSize));
+ int idx = index;
+ try (ClosableIterator<Map.Entry<byte[], byte[]>> iterator =
+ snapDiffReportTable.iterator(Optional.of(lowerIndex),
+ Optional.of(upperIndex))) {
+ while (iterator.hasNext() && diffReportList.size() < pageSize) {
Review Comment:
It would be nicer if you use counter `itemFetched` instead of using
`diffReportList.size()`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/RocksDbPersistentMap.java:
##########
@@ -85,9 +90,34 @@ public void remove(K key) {
}
@Override
- public ClosableIterator<Map.Entry<K, V>> iterator() {
- ManagedRocksIterator iterator =
- new ManagedRocksIterator(db.get().newIterator(columnFamilyHandle));
+ public ClosableIterator<Map.Entry<K, V>> iterator(Optional<K> lowerBound,
Review Comment:
Can you please add a unit test for this in
[TestRocksDbPersistentMap](https://github.com/apache/ozone/blob/69e3cf35058759447920796968e3b23cda7ee327/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestRocksDbPersistentMap.java#L47)?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/RocksDbPersistentMap.java:
##########
@@ -85,9 +90,34 @@ public void remove(K key) {
}
@Override
- public ClosableIterator<Map.Entry<K, V>> iterator() {
- ManagedRocksIterator iterator =
- new ManagedRocksIterator(db.get().newIterator(columnFamilyHandle));
+ public ClosableIterator<Map.Entry<K, V>> iterator(Optional<K> lowerBound,
+ Optional<K> upperBound) {
+ final ManagedReadOptions readOptions;
+ RocksIterator rocksIterator;
+ if (lowerBound.isPresent() || upperBound.isPresent()) {
+ readOptions = new ManagedReadOptions();
+ try {
+ if (lowerBound.isPresent()) {
+ readOptions.setIterateLowerBound(new ManagedSlice(
+ codecRegistry.asRawData(lowerBound.get())));
+ }
+
+ if (upperBound.isPresent()) {
+ readOptions.setIterateLowerBound(new ManagedSlice(
+ codecRegistry.asRawData(upperBound.get())));
+ }
+ } catch (IOException exception) {
+ // TODO: [SNAPSHOT] Fail gracefully.
+ throw new RuntimeException(exception);
+ }
+
+ rocksIterator = db.get().newIterator(columnFamilyHandle, readOptions);
+ } else {
+ readOptions = null;
Review Comment:
When `readOptions` is not provide, I guess RocksDB uses default
`readOptions` options. In that case we can just pass the `readOptions` all the
time. Limits will be set only if they are provided.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/RocksDbPersistentMap.java:
##########
@@ -20,12 +20,17 @@
import java.io.IOException;
import java.util.Map;
+import java.util.Optional;
+
import org.apache.hadoop.hdds.utils.db.CodecRegistry;
+import org.apache.hadoop.hdds.utils.db.managed.ManagedReadOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksIterator;
+import org.apache.hadoop.hdds.utils.db.managed.ManagedSlice;
import org.apache.hadoop.util.ClosableIterator;
import org.rocksdb.ColumnFamilyHandle;
import org.rocksdb.RocksDBException;
+import org.rocksdb.RocksIterator;
Review Comment:
Use of `RocksIterator` is restricted. Try to wrap it with
`ManagedRocksIterator` instead.
--
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]
