Complete BatchingTopicController refactoring: handler classes

[fvaleri]

Type of change

• Refactoring + fix

Description

This change completes the BatchingTopicController refactoring by introduicing a set of handlers, one for each external system, that host the request logic. With that, the BatchingTopicController class is only focused on the reconciliation logic, resulting in 40% less code and better maintenance.

Tested with all Topic Operator unit and integration tests, TopicST, and TopicReplicasChangeST. I also ran the scalability test comparing this branch with 0.41.0, and the performance doesn't change.

EDIT: This PR now includes a fix for the TO-CC race issue.

Checklist

• Make sure all tests pass
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally

[fvaleri]

Scalability comparison with the same custom perf test.

[fvaleri]

Failed test is unrelated and working when running locally.

[tombentley]

Now that we have a KafkaHandler and a KubernetesHandler shouldn't be be able to have unit tests specifically for those?

[fvaleri]

@tombentley this took more than I anticipated, as there were some test utils improvements to do. Mainly around recovery from interrupted tests, and namespace cleanup. I also improved the test log4j configuration. No existing test logic has been changed.

We now have tests for the two new handlers (Kube and Kafka), and I ran locally the full integration test suite multiple times, including TopicControllerIT, and a couple of system tests: TopicST, TopicReplicasChangeST. Finally, I also ran the Topic Operator manually and checked basic functionality.

I would suggests to run the regression tests suite (as I can't do it), and review this PR after #10241, so that I can take care of the rebase after the bugifx merge.

[fvaleri]

@ppatierno maybe you can start the regression tests. Thanks.

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tombentley]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[fvaleri]

@tombentley for now I created the addConfigurationWarnings to add back the config warning conditions, and enable related tests. We also need to refactor the CruiseControlHandler to avoid the KT status updates (side effect).

I'm a bit confused on how to avoid side effects in both cases with in-place recording. That's probably due to my lack of experience with functional programming. I know you suggested to use the new Results class, but how?

[fvaleri]

Completed the alter config filtering, now working on the replicas changes.

[fvaleri]

@tombentley following your suggestion, I removed side effects from the reconciliation logic, and added a note in BatchingTopicController.onUpdate. It is not perfect, but I think it's a big improvement, and should make maintenance easier. Feel free to iterate on that if you have time. I ran all tests mentioned above, plus manual and performance tests. It's all good. I'm going to ask to trigger regression tests one more time.

[tombentley]

This is now at the point of being too big to review properly. It was big before, but it now seems to combine what were two separate strands of work: One to fix the race condition and one to do a refactoring.

In many ways I 'd rather not merge something which it too big to review. But I know this already represents a significant effort, so I don't want to insist that you break it down into smaller PRs. But please let's avoid doing this again in the future!

[fvaleri]

@tombentley having separate PRs was the original plan, but then things started to pile up with time, and I didn't know the full solution anymore, so I needed all the pieces to make progress.

Even if I think the code is fine, I don't want to merge without your review. Would the following split work for you?

1. Test refactoring and cleanup.
2. Reconciliation refactoring.
3. TO-CC race condition issue fix.

[tombentley]

@fvaleri it would. If you're willing for break it up I'd appreciate it!

[fvaleri]

@Frawless can you please launch regression tests on this? Thanks.

[Frawless]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Frawless]

/packit test --labels regression

[fvaleri]

Regression tests looks good. We only have one failure in testing-farm from a flaky test, which is already reported in #10295.

[fvaleri]

Split into two new PRs.