You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2023/05/19 17:36:34 UTC
[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 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: 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