ableegoldman commented on code in PR #12773:
URL: https://github.com/apache/kafka/pull/12773#discussion_r1006804068


##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StoreChangelogReader.java:
##########
@@ -990,6 +991,10 @@ public void unregister(final Collection<TopicPartition> 
revokedChangelogs) {
         }
 
         removeChangelogsFromRestoreConsumer(revokedInitializedChangelogs);
+
+        if (changelogs.isEmpty()) {
+            state = ChangelogReaderState.ACTIVE_RESTORING;

Review Comment:
   Apologies if I'm missing something or my understanding of the 
restoration-related code is just completely out of date now, but do I 
understand correctly that this was/is a bug in the current (soon-to-be-old) 
architecture, and not related to the restoration thread work? 
   
   Also, just curious, did you find this bug due to you or someone else hitting 
it, or was it just something you happened to notice wasn't quite right? If 
you/someone is actually running into this "in the wild" it may be a good idea 
to file a jira ticket for it, just to document that the bug exists, we're aware 
of it, and that it's fixed in whichever versions.



##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##########
@@ -1070,6 +1071,11 @@ public void shouldTransitState() {
         assertEquals(ACTIVE_RESTORING, changelogReader.state());
         assertEquals(mkSet(tp, tp1, tp2), consumer.assignment());
         assertEquals(mkSet(tp1, tp2), consumer.paused());
+

Review Comment:
   Wow, there is a LOT going on in this test...encroaching on 180 lines, yikes. 
Could you split this out into its own test to make it more clear what is being 
tested/that we have coverage for this edge case -- it's hard to get a grasp on 
whether we're actually testing everything when it's all crammed into a single 
test.
   
   I know you didn't write this monolith to begin with, but we should strive 
always to improve on the sins of our ancestors 😉 More seriously though, 
covering multiple things in one test can be useful eg with integration tests 
where the setup & teardown alone take a significant chunk of time. But I would 
imagine/hope a unit test like this runs fairly quickly so it probably makes the 
most sense to optimize for comprehension
   
   Sorry for the slightly pedantic speech, if it's any consolation I bet Bruno 
would have said more or less the same thing lol



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