KAFKA-10357: Refactor makeReady to reduce complexity and support upcoming KIP-698 changes

[k-apol]

This PR reduces the cyclomatic complexity of the makeReady method by
extracting parts of its logic into smaller, well-named helper methods.
This structural cleanup makes the code easier to understand and
maintain.

These changes are intended to prepare for upcoming work related to
KIP-698,
which will introduce additional branching logic to makeReady.
Refactoring now ensures that future modifications can be made in a more
modular and testable way.

Reviewer: Matthias J. Sax [email protected]

[mjsax]

Doesn't validateTopics return a set of topic it could not validate, and still need to get created?

[k-apol]

The return value represents topics that are configured correctly, and that also need to get created

The method call here iterates the provided topics, and validates three things:

1. The topicsMap provided in the config knows about all of the topics we are trying to validate
2. The number of partitions for each internal topic is defined
3. If the internal topic already exists, we decide not to add it to the return set but validate the number of partitions it has matches what the InternalTopicManager has

If all 1 and 2 pass, we add to the return set, and repeat for all topics passed into the function

[mjsax]

The return value represents topics that are configured correctly, and that also need to get created

Well, the method does validate topics if they exist. That's why the method is called validateTopics -- but topics that do not exist, can by definition not be validated, but need to be created. So the returned set of topics, as you also confirmed (also need to get created), are non-existing topics, and thus, they did not get validated as there is nothing to be done ("validation" mean, checks if an existing topic meets the expected partition count).

Does this make sense?

[k-apol]

Yes.. I see your point, it may be a good opportunity to rename. If it's not properly validating the topics, I don't think we should name is validateTopics, this is throwing me off.

The purpose of this method is to verify the partition count of the existing topics and return topics that have not yet been created.. maybe findTopicsToCreate, determineTopicsToCreate, computeMissingTopics.. do any of these sound good to you?

[mjsax]

Well, the method does two things, validate existing topics and identify non-existing ones. So I think both names are ok, the current validateTopics or any of the names you proposed. As the method does two thing, there will be some side-effect. I am fine either way.

[mjsax]

nit: indention

[mjsax]

Should this go after the for-loop? I don't think we need to retry-backoff after verifying a single topic, but should be sufficient to check once for the whole "batch" of topics?

[mjsax]

It seems InternalTopicManagerTest fails. This refactoring should not change the logic though. Can you take a look? Can make another full pass, after the test is back to green :)