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