devmadhuu commented on code in PR #9904:
URL: https://github.com/apache/ozone/pull/9904#discussion_r2932543775
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStoreAbstractIterator.java:
##########
@@ -48,6 +56,20 @@ abstract class RDBStoreAbstractIterator<RAW>
this.rocksDBTable = table;
this.prefix = prefix;
this.type = type;
+ this.dbRef = acquireDbRef(table);
+ }
+
+ private static UncheckedAutoCloseable acquireDbRef(RDBTable table) {
+ if (table == null) {
+ return null;
+ }
+ try {
+ return table.acquireIterator();
+ } catch (RocksDatabaseException e) {
+ LOG.warn("Failed to acquire DB reference for iterator on table {}: {}",
+ table.getName(), e.getMessage());
+ return null;
Review Comment:
Actually here exception swallowed, , no reference counting guard ? Am I
missing something here ?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStoreAbstractIterator.java:
##########
@@ -155,5 +192,8 @@ public final void removeFromDB() throws
RocksDatabaseException, CodecException {
@Override
public void close() {
rocksDBIterator.close();
+ if (dbRef != null) {
+ dbRef.close();
Review Comment:
`dbRef` is final — it is never nulled out. If `close()` is called twice,
`dbRef.close()` runs twice. Since `dbRef` holds `counter::getAndDecrement`, the
counter gets decremented twice. Java's Closeable contract requires that calling
close() more than once is safe.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStoreAbstractIterator.java:
##########
@@ -155,5 +192,8 @@ public final void removeFromDB() throws
RocksDatabaseException, CodecException {
@Override
public void close() {
rocksDBIterator.close();
+ if (dbRef != null) {
+ dbRef.close();
+ }
Review Comment:
```suggestion
private final AtomicBoolean iteratorClosed = new AtomicBoolean(false);
@Override
public void close() {
if (iteratorClosed.compareAndSet(false, true)) {
rocksDBIterator.close();
if (dbRef != null) {
dbRef.close();
}
}
}
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStoreAbstractIterator.java:
##########
@@ -99,6 +131,9 @@ private void setCurrentEntry() {
@Override
public final boolean hasNext() {
+ if (isDbClosed()) {
Review Comment:
The `isClosed()` fast-fail only protects `hasNext().` Methods like
`seekToFirst(), seekToLast(), seek()` call native RocksDB directly without any
check.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
Review Comment:
nit: Please put a comment - "// Wait until all active operations (including
open iterators) complete.
// Iterators acquired after DB close is triggered will fast-fail in
// hasNext(), so this loop is expected to complete quickly in practice."
--
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]
