KAFKA-18913: Remove state.updater.enabled flag from StreamThread and …

[janchilling]

…related flow

This change removes the state.updater.enabled negation from
StreamThread, making the StateUpdater always active by default. As part
of this cleanup:

• Removed conditional code paths in StreamThread, ActiveTaskCreator,
  ProcessorStateManager, and StandbyTaskCreator.
• Removed the unused ChangelogReader, since it is no longer required in
  the execution flow.
• Updated affected tests (StreamThreadTest, ProcessorStateManagerTest,
  ActiveTaskCreatorTest, StreamThreadStateStoreProviderTest, etc.) to
  align with the new default behavior.

This is the first step in a series of PRs to fully eliminate the
state.updater.enabled configuration and simplify task management.

[janchilling]

Hi @cadonna , @mjsax ,

This is the first in a series of PRs to remove the state.updater.enabled flag, since it was requested to create multiple small PRs.

The StreamThread#initializeAndRestorePhase is no longer used, but I did not remove it yet because it is still referenced in TaskManagerTest. I plan to remove it in a follow-up PR once those tests are updated.

For updating the TaskManagerTest, I may need some help to ensure the changes are done correctly.

If there are any adjustments you’d like me to make in this PR, or if the way I am approaching this series is not ideal, please let me know.

[mjsax]

@janchilling -- You did go silent on your other PR, and I am just seeing this PR now. We thought you did loose interest and @shashankhs11 started to pickup this work....

I have no personal preference, but would kindly ask you and @shashankhs11 to agree on who will complete this work. -- Sorry for all this mix up, but you did go silent on the other PR :/

\cc @lucasbru

[shashankhs11]

I am happy to hand this work over to @janchilling completely as they were the first to work on this and I think it would not be fair for me to continue.

However, I believe that I have a good understanding of the work that is required along with a systematic approach after having discussed this in detail offline. I would love to continue to work on this task but only if @janchilling hands it over to me.

I would like to leave it to @janchilling to ultimately make the decision.

[lucasbru]

Sounds good to me. Once @janchilling confirms taking on the whole work, we can close the other PRs. Note that completing the work does not only involve removing code, but also porting / reimplementing some tests that are currently only covered on the non-state-updater code path. It would be good to commit to completing the whole work, including those tests.

[janchilling]

Hi @mjsax , @lucasbru , @shashankhs11 ,

I’m happy to share this PR with @shashankhs11, and it would be great if I can collaborate with him on this.

The main place I got stuck was while updating the test cases in the TaskManagerTest class. I don’t fully understand some of the functionalities being tested there. That was actually the only remaining part left in the earlier PR. I’ve completed everything else except updating the TaskManagerTest and one other version test.

It would be good to commit to completing the whole work, including those tests.

Since the Earlier PR (Excuse my messiness) became quite large, I believe it was suggested to split it into smaller ones. So this is the first in a series of PRs I planned to create.

Again, I’d be happy to share this with @shashankhs11 and contribute in any way that’s helpful.

[shashankhs11]

That would be awesome! I would really love to collaborate and help resolve the issue. However, I think collaborating might be a difficult task to tackle, but I've got a plan.

I got some insights from @mjsax a few weeks ago on how we can approach this. I believe that this PR is still large and might be harder to review.

Therefore, I would like to propose that we come up with an initial detailed plan (but flexible) wherein we can split the main task into smaller tasks. I would like to take the ownership of coming up with this detailed plan and will share an initial draft plan as soon as possible. You can then review the plan and add/remove details. If we can target to finalize a plan within the next few days, that would be awesome.

Once we finalize a plan, we can then work on them and I believe it would also make it easier and quicker for @lucasbru to review smaller PRs.

Does this sound good to you? Please feel free to give your insights on this idea.

[mjsax]

If you want to divide the work, a good approach would be to first split up the work to update all integrations test, before we merge any actual code changes?

[github-actions]

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

[shashankhs11]

a good approach would be to first split up the work to update all integrations test, before we merge any actual code changes

from my analysis, I have identified these integration tests that requires update.

• RestoreIntegrationTest.java
• EosIntegrationTest.java
• SmokeTestDriverIntegrationTest
• PauseResumeIntegrationTest
• KafkaStreamsTelemetryIntegrationTest

I think it would be best for us if we split this updates into 2 seperate tasks for each of us to work on

Task 1

• RestoreIntegrationTest.java KAFKA-19666: Clean up integration tests related to state-updater #20498
• EosIntegrationTest.java KAFKA-19666: Clean up integration tests related to state-updater #20499

Task 2

• SmokeTestDriverIntegrationTest KAFKA-19666: Clean up integration tests related to state-updater #20465
• PauseResumeIntegrationTest KAFKA-19666: Clean up integration tests related to state-updater #20463
• KafkaStreamsTelemetryIntegrationTest KAFKA-19666: Clean up integration tests related to state-updater #20462

We can further also have one PR per integration test to make it cleaner and easier to review.

I can pickup Task 2 to get started with and @janchilling can pick up Task 1? Does this sound like a good plan? @mjsax @lucasbru

Please add if I have missed anything.

[lucasbru]

Cleaning up integration tests sounds good. However, before starting to actually removing the production code path, I would suggest starting with the tricky bit, and then working your way to the easier parts. We don't want to remove half of the old code path first, to then realize that we cannot figure out how to preserve test coverage without the old code path.

IIRC the "tricky bit" that @janchilling was stuck with is porting the tests that use tryToCompleteRestoration in TaskManagerTest to not use tryToCompleteRestoration (because it will be removed). I think, for the most part, the function is only used to advance task state, and it should be possible to replace it by using checkStateUpdater and mocking the stateUpdater mock correctly. We can also remove tests if we can point to an existing test that tests the equivalent state updater code path.

Once we have solved this, the rest should just be removing code.

[github-actions]

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

[shashankhs11]

I created a new subtask KAFKA-19666 to this parent issue to track the cleanup of all related integration tests.

@lucasbru clean up of the below tests are ready for review

• KafkaStreamsTelemetryIntegrationTest.java (KAFKA-19666: Clean up integration tests related to state-updater #20462)
• PauseResumeIntegrationTest.java (KAFKA-19666: Clean up integration tests related to state-updater #20463)
• SmokeTestDriverIntegrationTest.java (KAFKA-19666: Clean up integration tests related to state-updater #20465)

[github-actions]

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

[shashankhs11]

I can pickup Task 2 to get started with and @janchilling can pick up Task 1?

Since, we did not hear back yet, I decided to proceed with the cleanup of all integration tests.

@lucasbru cleanup of the remaining integration tests are available for review. This should complete the subtask KAFKA-19666