[GitHub] [kafka] lihaosky commented on a diff in pull request #12638: Register and unregister changelog topics in state updater

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

lihaosky commented on code in PR #12638:
URL: https://github.com/apache/kafka/pull/12638#discussion_r971383169


##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/ChangelogRegister.java:
##########
@@ -31,6 +33,8 @@ public interface ChangelogRegister {
      */
     void register(final TopicPartition partition, final ProcessorStateManager stateManager);
 
+    void register(final Set<TopicPartition> partition, final ProcessorStateManager stateManager);

Review Comment:
   nit: partition -> partitions?



##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -444,6 +451,8 @@ public void restore(final Map<TaskId, Task> tasks) {
                 final Set<TaskId> corruptedTasks = new HashSet<>();
                 e.partitions().forEach(partition -> corruptedTasks.add(changelogs.get(partition).stateManager.taskId()));
                 throw new TaskCorruptedException(corruptedTasks, e);
+            } catch (final InterruptException interruptException) {
+                throw interruptException;

Review Comment:
   QQ: will `InterruptException` be thrown even without this?



##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -347,6 +348,12 @@ public void register(final TopicPartition partition, final ProcessorStateManager
         }
     }
 
+    public void register(final Set<TopicPartition> changelogPartitions, final ProcessorStateManager stateManager) {

Review Comment:
   nit: `@Override`



##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorStateManager.java:
##########
@@ -209,7 +212,9 @@ void registerStateStores(final List<StateStore> allStores, final InternalProcess
         processorContext.uninitialize();
         for (final StateStore store : allStores) {
             if (stores.containsKey(store.name())) {
-                maybeRegisterStoreWithChangelogReader(store.name());
+                if (!stateUpdaterEnabled) {
+                    maybeRegisterStoreWithChangelogReader(store.name());

Review Comment:
   QQ: this method is called `registerStateStores` but why does it expect `store` to be already in `stores`? The only place I can find `stores.put` is called is in `registerStore` and in that method, `maybeRegisterStoreWithChangelogReader` is called immediately after `stores.put` is called. So I'm confused what's the real purpose of this method and if `maybeRegisterStoreWithChangelogReader` call here is redundant.



-- 
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-unsubscribe@kafka.apache.org

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