Consolidating the KafkaRebalance API for extensibility

[kyguy]

This proposal addresses the increasing complexity and limited extensibility of the KafkaRebalance custom resource API.

[ppatierno]

@kyguy is this ready to review? Should we add maintainers or are you still working on it?

[kyguy]

@ppatierno yes, this is now ready for review! I'll request the maintainers as reviewers now

[im-konge]

Thanks for the proposal. I left few comments.

[PaulRMellor]

The proposal is clear and well justified. I’ve added a few minor suggestions as I read through

[kyguy]

This is ready for another round of reviews whenever people have time of course!

[ppatierno]

I think we could even log a warning that they are both set but the old form will be ignored.

[ppatierno]

Also what happens in the following scenario?

spec:           
    concurrentPartitionMovementsPerBroker: 10  # old                                                                                                                                                                   
    config:                                         
      concurrent_leader_movements: "5"  # new but different field 

Is the logic going to ignore the old concurrentPartitionMovementsPerBroker because the resource has a spec.config field or it's going to make a merge because they are referring to two different fields?

[kyguy]

It would make a merge, respecting both fields and values but a warning would still be logged for the usage of the old concurrentPartitionMovementsPerBroker form.

[kyguy]

Just updated some text here in to address the first comment in the latest commit, let me know what you think!

[ppatierno]

I would expect a merge yes, but at the same time, if our goal is discourage the usage of the current properties in favor of the config section, we could also decide that if you set the config section, it's going to override everything else coming from the dedicated fields. It could be counterintuitive though. In the past we had something similar with templates across Kafka and KafkaNodePool and we moved from an override approach to a merge. Let's see what others think about this.

[kyguy]

Updated the proposal following Shubham's suggestion with the latest commit. Take a look when you get a chance and let me know what you think!

[kyguy]

From Jakub's feedback below, I believe he may be favor of a merge as well

Mixing of new and old fields is allowed
Deprecation warnings are issues automatically, no need to do anything about them
If both fields are set, new fields takes priority, but you can issue a warning that they conflict.

This is another place I would like to get consensus on, whether:

(A) We merge the fields
(B) We only allow all new API fields or all old API fields.

I am leaning towards (B) but I don't mind doing (A). Given the discussion above do people have a strong preference on this? I think both options have good justifications but I want to agree on a single option/direction for this so we can move this forward.

CC @ppatierno @ShubhamRwt @tinaselenge @katheris @scholzj any strong opinions for or against?

[ShubhamRwt]

+1 for (B) as it keeps things more clear and avoid any sort of complex. merge logic

[scholzj]

There is no complex logic. Each old field should have a clear mapping to a new field. We should handle this in the same way how we handled the previous API changes:

• Take the resource on the beginning of the reconciliation
• Check the old fields
• If the old field is set, check the new field
• If the new field is empty, move the value from old field there
• If the new field is also set, raise a warning and ignore the old value
• Continue with the reconciliation

That is the standard process we follow. Last used for example in the MirrorMaker 2 and Connect API changes and other v1 API related changes.

[kyguy]

Each old field should have a clear mapping to a new field. We should handle this in the same way how we handled the previous API changes

That is the standard process we follow. Last used for example in the MirrorMaker 2 and Connect API changes and other v1 API related changes.

These are a fair points, the transitions of those went pretty smoothly as I remember and the logic added was easy to remove later too. I have updated the proposal to follow this process, citing Mirror Maker 2 and Connect API changes as precedents.

Updates are in the latest commit.

[scholzj]

Thanks for the proposal. I left some comments.

[tinaselenge]

Thank you for the proposal @kyguy. I think the proposal makes sense and the format of KafkaRebalance resource does look better, instead of crowding it with many more API fields. I left a comment around how we should document these changes, without making users more confused.

[im-konge]

@kyguy there were some discussions and review comments, are you going to come back to this? Or is there anything blocking this proposal?

[kyguy]

are you going to come back to this? Or is there anything blocking this proposal?

Nothing blocking this proposal at the moment, I still have some feedback to address, I will be getting back to it this and next week!

[kyguy]

This proposal is ready for another round of reviews!

I've gone through the proposal and either responded to or addressed every open comment. If the changes don't address your feedback, let me know and I'll revisit them.

[ShubhamRwt]

I had a first pass. The proposal looks good to me. Just left some questions and comments

[kyguy]

Thanks for the feedback, @ShubhamRwt. I like the idea you suggested, it offers a solution to one of the open questions we have in another part of the proposal, so I've linked it there for further discussion. Let's see what the maintainers think and hopefully we can agree on a direction!

[fvaleri]

Hi Kyle, this is a nice improvement and the general direction makes sense.

A few suggestions and considerations:

Why not use a single top level field instead of reusing brokers and introducing volumes? This would avoid broker ID duplication when deprecated fields are eventually removed.

  targetBrokers:
    - nodeId: 0
      volumeIds: [0, 1, 2] # optional
    - nodeId: 1
      volumeIds: [0, 1]    # optional

The NotReady approach for mixed old/new usage is the right call. No merge logic, no precedence ambiguity. Clean contract.

Moving API parameters to a config map causes loss of type safety. This is the same tradeoff that other components made to get full extensibility, but it should be called out explicitly.

On the validation side, the strategy is pass through and relay CC errors via status conditions. As a future enhancement, it would be great if CC endpoint could expose the parameters' schema to be cached and used for validation.

The proposal deprecates old fields but does not say when they will be removed. Will it be in the next major release? Will we provide a conversion tool like we did for 1.0? I think we should state the plan explicitly.

[scholzj]

@kyguy I left some more comments. I think the implementation deserves a bit more refinement. I should probably use the same process as common in other similar situations:

• At the beginning of the reconciliation, you convert the old fields to new fields (in the future, a gatekeeper plugin could be used for that)
• The fields are converted one by one independently
• Mixing of new and old fields is allowed
• Deprecation warnings are issues automatically, no need to do anything about them
• If both fields are set, new fields takes priority, but you can issue a warning that they conflict.
• You do this conversion once at the beginning and the rest of the code uses only the new APIs.

[kyguy]

In response to @fvaleri

Why not use a single top level field instead of reusing brokers and introducing volumes? This would avoid broker ID duplication when deprecated fields are eventually removed.

I actually considered something like this early on but thought it best to keep brokers since its simple and is used in other resources like in the BrokerCapacityOverride section of the Kafka resource. Also, there shouldn't be any broker ID duplication since brokers and moveReplicasOffVolumes (renamed to volumes) are mutually exclusive, no mode uses both fields simultaneously. So after the deprecated fields are removed, there wouldn't be any overlap.

The NotReady approach for mixed old/new usage is the right call. No merge logic, no precedence ambiguity. Clean contract.

+1, just made changes in latest commit.

Moving API parameters to a config map causes loss of type safety. This is the same tradeoff that other components made to get full extensibility, but it should be called out explicitly.

+1, just made changes in latest commit.

On the validation side, the strategy is pass through and relay CC errors via status conditions. As a future enhancement, it would be great if CC endpoint could expose the parameters' schema to be cached and used for validation.

+1, just added this in the Future Extensibility section in latest commit.

The proposal deprecates old fields but does not say when they will be removed. Will it be in the next major release? Will we provide a conversion tool like we did for 1.0? I think we should state the plan explicitly.

+1, just made changes to specify this in latest commit.

[tinaselenge]

Thank you for the proposal @kyguy. I didn't have any additional comments on the top of what others said and they all seem to be addressed now. So, this looks good to me now. I see there is an ongoing discussion around brokers and volumes fields, how to join them. I'm kind of in-between with this but leaning more towards the way you currently proposed, keeping them as separate fields. These fields are specific to different modes and are never used together. When removing/adding brokers, I find the current brokers field that takes a list of broker Ids, is simpler. And then removing/demoting disks, has a dedicated volumes field is also simpler even though it also takes list of broker Ids. The current structure makes the intention clear and the validation simpler (e.g. brokers field is only required for remove/add brokers mode). So I'm happy if you stick with the current design, but if the consensus goes toward joining them, I'm fine with that too.