[WIP] Support intra-broker throttling on rebalance (default.log.dir.throttle)

[dongjinleekr]

Type of change

• Enhancement / new feature
• Documentation

Description

Add logDirThrottle to KafkaRebalanceSpec, which will be forwarded into CruiseControl's default.log.dir.throttle configuration and log_dir_throttle parameter. (#11327)

Checklist

• [-] Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• [-] Supply screenshots for visual changes, such as Grafana dashboards

[dongjinleekr]

Resolves #11327

This PR is WIP as of present. For this PR to be tested, the Cruise Control's PR should be merged beforehand + This is my first contribution and I am still troubled with building & testing. 😓 (The annotation processing in my IDE is not working! HELP! 😱)

+1. Do I need to file a proposal for this feature? 🤔

[ppatierno]

Thanks for the PR :-)
I see this PR related to your issue #11327 (which you should mention in the description for clarity).
As you said the CC related PR is not merged yet and also CC is not released yet including that improvement and the coming Strimzi 0.46.0 will already including CC 2.5.142. Can you mark this PR as a "draft" please?
I also think that a proposal would be needed. I don't expect it to be big. @strimzi/maintainers thoughts?

[scholzj]

I also think that a proposal would be needed. I don't expect it to be big. @strimzi/maintainers thoughts?

@ppatierno This seems pretty straightforward in mirroring the existing inter-broker throttle configuration. So while it changes the API, I personally do not think it needs a proposal. Do you see any other implications of this new option that I'm missing?

[ppatierno]

@scholzj I don't see big implications but the way I look at Strimzi proposal is about having one when there is a change on the API (where by change I also mean a new addition). Even this simple one https://github.com/strimzi/proposals/blob/main/095-add-support-volumeattributesclassname.md had a proposal for example.

[scholzj]

@scholzj I don't see big implications but the way I look at Strimzi proposal is about having one when there is a change on the API (where by change I also mean a new addition). Even this simple one https://github.com/strimzi/proposals/blob/main/095-add-support-volumeattributesclassname.md had a proposal for example.

For me, the difference is that the Volume Attribute Class is a brand new feature with new behavior (while we have Storage Class, that has a different purpose, is immutable etc.). This is for me pretty much the same as the existing option for inter-broker throttling. So I do not feel this needs a proposal. (But of course, I do not problem with having a proposal for it - just sharing my thoughts as you asked for 😉)

[ppatierno]

Absolutely, for this reason I asked for others' opinions. Not having the proposal would simplify the process a lot for having the feature done asap (when it's in CC of course). I am fine even without it. Let's say that "when having a proposal" is kind of ambiguous or unclear unless it's a really big new feature which needs a place for discussion. Not sure it could be the same for the community as well ... but of course it's a different problem.

[im-konge]

Sorry I missed the PR. In this case, I would keep it as it is - if it's relatively small change - and I wouldn't require proposal.

[dongjinleekr]

@ppatierno @scholzj @im-konge

Thanks for the comments. I just updated the title with '[WIP] and the description with issue. Plus, I succeeded to fix the IDE problem and fixd the broken test. 😄

It seems like this PR does not require any Proposal. So, Let me notify you as soon as the original feature is closed. 😉

[ppatierno]

@dongjinleekr thanks for the PR! To start you should ...

1. fix the DCO (you can find more info if you click on the ... on the right and view details)
2. fix the build error due to the following

./.azure/scripts/check_docs.sh
Replace 'i.e'. with 'that is, ':
documentation/modules/appendix_crds.adoc:4311:|The upper bound, in bytes per second, on the bandwidth used to move replicas between brokers (i.e., inter-broker replica movements). There is no limit by default.
documentation/modules/appendix_crds.adoc:4314:|The upper bound, in bytes per second, on the bandwidth used to move replicas between disks (i.e., intra-broker replica movements). There is no limit by default.
documentation/modules/cruise-control/con-rebalance-performance.adoc:74:| The bandwidth (in bytes per second) to assign to inter-broker partition reassignment (i.e., replica movements between brokers)
documentation/modules/cruise-control/con-rebalance-performance.adoc:79:| The bandwidth (in bytes per second) to assign to intra-broker partition reassignment (i.e., replica movements between disks)
ERROR: 4 docs problems found.

It would be also great having a mention for this addition in the CHANGELOG.

Finally, if you think the PR is ready, you can remove the [WIP] prefix from the title :-)