mjsax commented on code in PR #13142:
URL: https://github.com/apache/kafka/pull/13142#discussion_r1084826103


##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java:
##########
@@ -114,6 +114,7 @@ public class RocksDBStore implements KeyValueStore<Bytes, 
byte[]>, BatchWritingS
     private boolean userSpecifiedStatistics = false;
 
     private final RocksDBMetricsRecorder metricsRecorder;
+    private final boolean userManagedIterators;

Review Comment:
   "User" sounds a little bit like KS user to me. Maybe we can rename, eg, 
`selfManagedIterators` or similar (not sure what a good name would be).



##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java:
##########
@@ -351,13 +360,23 @@ public <R> QueryResult<R> query(
     @Override
     public <PS extends Serializer<P>, P> KeyValueIterator<Bytes, byte[]> 
prefixScan(final P prefix,
                                                                                
     final PS prefixKeySerializer) {
+        if (userManagedIterators) {
+            throw new IllegalStateException("Must specify openIterators in 
call to prefixScan()");
+        }
+        return prefixScan(prefix, prefixKeySerializer, openIterators);
+    }
+
+    <PS extends Serializer<P>, P> KeyValueIterator<Bytes, byte[]> 
prefixScan(final P prefix,
+                                                                             
final PS prefixKeySerializer,
+                                                                             
final Set<KeyValueIterator<Bytes, byte[]>> openIterators) {

Review Comment:
   Is it valid to call this method if `userManagedIterators` is false? (Similar 
elsewhere.)



##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBTimestampedStore.java:
##########
@@ -383,7 +383,9 @@ public KeyValue<Bytes, byte[]> makeNext() {
 
         @Override
         public synchronized void close() {
-            openIterators.remove(this);
+            if (closeCallback != null) {

Review Comment:
   Don't we require that we always have a registered `closeCallback`? If yes, 
it seems invalid that it's `null` and having this check might mask a bug, and 
thus we should rather not have it, but let it crash with a NPE? (Also 
elsewhere.)



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to