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 :-)

[katheris]

@dongjinleekr if this PR is ready for review can you also mark it ready by clicking the button, as it's still a draft PR currently

[ppatierno]

@dongjinleekr are you still planning to work on this PR, fixing the DCO and the build issue and move out from Draft? I am asking because we would like trying to have a 0.46.0 RC1 by the end of this week but it means that this PR should be reviewed (and approved) soon otherwise it will move to next 0.47.0 release.

[scholzj]

@ppatierno @katheris This is waiting for the Cruise Control support to be merged and released first.

[ppatierno]

@scholzj Right, I forgot. This is the PR in CC which is still not merged linkedin/cruise-control#2145 Anyway after the CC PR is merged we need to wait for a CC release and then using that version within the operator. It could take too long and in such a casa we'll move this PR to 0.47.0 milestone.