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
