[GitHub] [kafka] vvcephei commented on a change in pull request #8676: KAFKA-10005: Decouple RestoreListener from RestoreCallback

[GitBox <gi...@apache.org>]

vvcephei commented on a change in pull request #8676:
URL: https://github.com/apache/kafka/pull/8676#discussion_r436212253



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/CompositeRestoreListener.java
##########
@@ -1,116 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more

Review comment:
       _awesome_

##########
File path: streams/src/main/java/org/apache/kafka/streams/StreamsBuilder.java
##########
@@ -38,7 +38,6 @@
 import org.apache.kafka.streams.processor.internals.ProcessorNode;
 import org.apache.kafka.streams.processor.internals.SourceNode;
 import org.apache.kafka.streams.state.KeyValueStore;
-import org.apache.kafka.streams.state.ReadOnlyKeyValueStore;

Review comment:
       weird that the import became unused, even though there were no other code changes...

##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java
##########
@@ -210,30 +207,6 @@ public void shouldFindSingleStoreForChangelog() {
         );
     }
 
-    @Test
-    public void shouldRestoreStoreWithRestoreCallback() {

Review comment:
       It doesn't look like this was related to the bulk loading, but I'm guessing it was.

##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBStoreTest.java
##########
@@ -237,25 +236,6 @@ public void shouldNotRemoveStatisticsFromInjectedMetricsRecorderOnCloseWhenUserP
         verify(metricsRecorder);
     }
 
-    @Test
-    public void shouldRespectBulkloadOptionsDuringInit() {
-        rocksDBStore.init(context, rocksDBStore);
-
-        final StateRestoreListener restoreListener = context.getRestoreListener(rocksDBStore.name());
-
-        restoreListener.onRestoreStart(null, rocksDBStore.name(), 0L, 0L);
-
-        assertThat(rocksDBStore.getOptions().level0FileNumCompactionTrigger(), equalTo(1 << 30));

Review comment:
       Were the bulk loading options a public interface? It doesn't seem like ignoring them would be a "breakage", so I don't think it hinders the removal, but it does seem worth documenting, and maybe trying to detect and warn?




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

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