KAFKA-18913: Removing _state.updater.enabled_ flag through the Stream… #19275
Conversation
…Thread#runOnceWithoutProcessingThreads flow. Removed code of the method StreamThread#initializeAndRestorePhase, since it was only used in the negation flow. Also removed the flag entirely from the StreamThread#runOnceWithoutProcessingThreads method.
|
Hi @cadonna , Would you be able to verify if the changes I have done are correct, like is this what is required? Also TaskManager#needsInitializationOrRestoration had only one usage through StreamThread#initializeAndRestorePhase method (which was removed). And TaskManager#tryToCompleteRestoration also had only one usage but lot of test cases around 53 I guess. So wanted to know if I should go levels below and remove the code or just top level code. I will do the changes to remove the flag from the other places and commit them, in the meantime would be great if you can verify if my approach is correct. Also could you also start the workflow as well? |
…Thread#create flow. Removed the stateUpdaterEnabled dependency completely from the StreamThread#create methods and all the inner methods that use it
…Thread#maybeGetClientInstanceIds, StreamThread#pollPhase and StreamThread#clientInstanceIds flow. Removed the stateUpdaterEnabled dependency completely from the StreamThread#maybeGetClientInstanceIds, StreamThread#pollPhase and StreamThread#clientInstanceIds flow. Also with this commit, have removed the flag dependency completely from the StreamThread and StreamThreadTest classes
There was a problem hiding this comment.
Thanks for the PR, @janchilling !
I started the review, but was not able to finish it. Nevertheless, I would like to share the comments up until now with you. Most comments are about additional indentation. We use 4 spaces and not 8. There are also some other comments about the code.
I will proceed with the review next week. Feel free to update the PR in the meanwhile.
| @@ -372,15 +371,13 @@ public static StreamThread create(final TopologyMetadata topologyMetadata, | |||
| final Runnable shutdownErrorHook, | |||
| final BiConsumer<Throwable, Boolean> streamsUncaughtExceptionHandler) { | |||
|
|
|||
| final boolean stateUpdaterEnabled = InternalConfig.stateUpdaterEnabled(config.originals()); | |||
There was a problem hiding this comment.
Could you please also remove the internal config _state.updater.enabled_ and all corresponding code from StreamsConfig?
| "only DEAD state is a valid next state", newState); | ||
| "only DEAD state is a valid next state", newState); |
There was a problem hiding this comment.
Could you revert this change, please?
| "no valid next state after DEAD", newState); | ||
| "no valid next state after DEAD", newState); |
There was a problem hiding this comment.
Could you revert this change, please?
| time, | ||
| config, | ||
| restorationLogContext, | ||
| adminClient, | ||
| restoreConsumer, | ||
| userStateRestoreListener, | ||
| userStandbyUpdateListener | ||
| time, | ||
| config, | ||
| restorationLogContext, | ||
| adminClient, | ||
| restoreConsumer, | ||
| userStateRestoreListener, | ||
| userStandbyUpdateListener |
There was a problem hiding this comment.
Could you please revert this change?
| topologyMetadata, | ||
| config, | ||
| streamsMetrics, | ||
| stateDirectory, | ||
| changelogReader, | ||
| cache, | ||
| time, | ||
| clientSupplier, | ||
| threadId, | ||
| threadIdx, | ||
| processId, | ||
| log, | ||
| proceessingThreadsEnabled |
There was a problem hiding this comment.
Could you please revert the additional indentation?
| @@ -831,7 +816,7 @@ void maybeGetClientInstanceIds() { | |||
| if (fetchDeadlineClientInstanceId >= time.milliseconds()) { | |||
| try { | |||
| producerInstanceIdFuture.complete( | |||
| taskManager.streamsProducer().kafkaProducer().clientInstanceId(Duration.ZERO) | |||
| taskManager.streamsProducer().kafkaProducer().clientInstanceId(Duration.ZERO) | |||
There was a problem hiding this comment.
Could you please revert the additional indentation?
| @@ -844,14 +829,14 @@ void maybeGetClientInstanceIds() { | |||
| } | |||
| } else { | |||
| producerInstanceIdFuture.completeExceptionally( | |||
| new TimeoutException("Could not retrieve thread producer client instance id.") | |||
| new TimeoutException("Could not retrieve thread producer client instance id.") | |||
There was a problem hiding this comment.
Could you please revert the additional indentation?
| && restoreConsumerInstanceIdFuture.isDone() | ||
| && producerInstanceIdFuture.isDone()) { |
There was a problem hiding this comment.
Could you please revert the additional indentation?
| ); | ||
| } | ||
| } | ||
|
|
||
| if (mainConsumerInstanceIdFuture.isDone() | ||
| && (!stateUpdaterEnabled && restoreConsumerInstanceIdFuture.isDone()) | ||
| && producerInstanceIdFuture.isDone()) { | ||
| && restoreConsumerInstanceIdFuture.isDone() |
There was a problem hiding this comment.
I believe you can also remove this line. With the state updater fetching this instance ID is done within the state updater.
| "This implies that this thread missed a rebalance and dropped out of the consumer group. " + | ||
| "Will close out all assigned tasks and rejoin the consumer group.", e); | ||
| "This implies that this thread missed a rebalance and dropped out of the consumer group. " + | ||
| "Will close out all assigned tasks and rejoin the consumer group.", e); |
There was a problem hiding this comment.
Could you please revert the additional indentation?
…Thread#runOnceWithoutProcessingThreads flow.
Removed code of the method StreamThread#initializeAndRestorePhase, since it was only used in the negation flow. Also removed the flag entirely from the StreamThread#runOnceWithoutProcessingThreads method.
Will remove the flag and the related code entirely from the future commits!