Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete BatchingTopicController refactoring: handler classes #10248

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Jun 20, 2024

Type of change

  • Refactoring

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.

Checklist

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

@fvaleri fvaleri requested a review from tombentley June 20, 2024 17:17
@fvaleri fvaleri added this to the 0.42.0 milestone Jun 20, 2024
@fvaleri fvaleri requested a review from ppatierno June 20, 2024 17:18
@fvaleri
Copy link
Contributor Author

fvaleri commented Jun 20, 2024

Scalability comparison with the same custom test.

Screenshot from 2024-06-20 19-10-57

@fvaleri
Copy link
Contributor Author

fvaleri commented Jun 21, 2024

Failed test is unrelated and working when running locally.

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Signed-off-by: Federico Valeri <[email protected]>
@fvaleri fvaleri force-pushed the to-btc-ref-handlers branch 5 times, most recently from 4ccda8a to cad9cde Compare July 1, 2024 08:47
Signed-off-by: Federico Valeri <[email protected]>
@scholzj scholzj modified the milestones: 0.42.0, 0.43.0 Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants