[GitHub] [kafka] ableegoldman commented on a change in pull request #11813: KAFKA-12648: fix #add/removeNamedTopology blocking behavior when app is in CREATED

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

ableegoldman commented on a change in pull request #11813:
URL: https://github.com/apache/kafka/pull/11813#discussion_r817412782



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/namedtopology/KafkaStreamsNamedTopologyWrapper.java
##########
@@ -230,53 +233,72 @@ public RemoveNamedTopologyResult removeNamedTopology(final String topologyToRemo
 
         topologyMetadata.unregisterTopology(removeTopologyFuture, topologyToRemove);
 
-        if (resetOffsets) {
+        if (!completedFutureForUnstartedApp(removeTopologyFuture, "removing topology") && resetOffsets) {

Review comment:
       Oh mm actually this change is what's causing the tests to hang as it breaks the fix -- we actually need to ensure that we check the CREATED state and complete the future if so. But while I still stand by my comments above, ie that trying to avoid entering a synchronized block when adding or removing a topology is probably premature optimization, I actually did look over the class and believe we can make this work without synchronizing this particular method.
   
   (However we do still need to synchronize on `start`, for several reasons)




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