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.

Sign up for GitHub

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

Jump to bottom

Refactor the implementation for getting status of rebalance proposal request #10607

Merged
merged 3 commits into from
Oct 3, 2024
Merged

Refactor the implementation for getting status of rebalance proposal request #10607

merged 3 commits into from
Oct 3, 2024

Conversation

tinaselenge
Copy link
Contributor

Type of change

Select the type of your PR

  • Refactoring

Description

  • Use /kafkacruisecontrol/user_tasks endpoint to check the status of proposal request (rebalance request in dry mode) instead of /kafkacruisecontrol/rebalance endpoint with userTaskId. This makes it more consistent with how we check the status of rebalance request.

  • Updated MockCruiseControl.setupCCRebalanceResponse() to set user task id to the one that matches verbose response, so that the subsequent requests to /user_tasks endpoint can return a verbose response with load optimisation.

Closes #10294

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • 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

@scholzj scholzj requested review from ppatierno and removed request for ppatierno September 19, 2024 12:17
@scholzj scholzj added this to the 0.44.0 milestone Sep 19, 2024
@tinaselenge tinaselenge marked this pull request as ready for review September 24, 2024 08:29
@scholzj
Copy link
Member

scholzj commented Sep 24, 2024

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM assuming the tests pass. Should be reviewed by SMEs (@ppatierno )

ppatierno
ppatierno reviewed Sep 28, 2024
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

@tinaselenge I left one comment and overall it seems to be ok. Anyway before merging I would love if you could rebase against the main (where now the auto-rebalancing is available). I would like to give this PR a go locally together with auto-rebalancing before approving.

.../main/java/io/strimzi/operator/cluster/operator/assembly/KafkaRebalanceAssemblyOperator.java Show resolved Hide resolved
@tinaselenge tinaselenge force-pushed the refactor-rebalancing branch from 5d33f2b to bf7827c Compare October 1, 2024 11:06
@tinaselenge
Copy link
Contributor Author

@tinaselenge I left one comment and overall it seems to be ok. Anyway before merging I would love if you could rebase against the main (where now the auto-rebalancing is available). I would like to give this PR a go locally together with auto-rebalancing before approving.

@ppatierno I have rebased it to main :)

@ppatierno
Copy link
Member

@tinaselenge thanks! Anyway I would like to wait for this PR #10652 to be merged and doing another rebase, because it fixes a subtle bug in auto-rebalancing. So I would like to try your PR against the "final" auto-rebalancing. But in general your PR LGTM, just looking for the confirmation by testing it with the auto-rebalancing.

@ppatierno
Copy link
Member

ppatierno commented Oct 1, 2024 •
edited
Loading

@tinaselenge finally #10652 was merged. Can you rebase your PR so I can try it locally? Thanks!

@tinaselenge
Refactor the implementation for getting status of rebalance proposal …
86cf827 
…request

Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Refactor getUserTaskStatus with a common method for handling the resp…
cc78c9c 
…onse for both proposal and rebalance request

Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge tinaselenge force-pushed the refactor-rebalancing branch from bf7827c to cc78c9c Compare October 2, 2024 11:03
@tinaselenge
Address review comment
6323115 
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
@tinaselenge
Copy link
Contributor Author

@ppatierno I have rebased again and addressed the review comment. Thanks

@ppatierno
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno
ppatierno approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thanks!
Also tested within the auto-rebalancing scenario and it seems to work.

@ppatierno ppatierno merged commit 2f1193a into strimzi:main Oct 3, 2024
21 checks passed
@tinaselenge tinaselenge deleted the refactor-rebalancing branch October 4, 2024 10:08
@tinaselenge tinaselenge mentioned this pull request Oct 22, 2024
Closed
8 tasks
@tinaselenge tinaselenge mentioned this pull request Oct 29, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShubhamRwt ShubhamRwt ShubhamRwt approved these changes

@ppatierno ppatierno ppatierno approved these changes

@scholzj scholzj scholzj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.44.0
Development

Successfully merging this pull request may close these issues.

Getting user task status of getting proposal and current rebalancing should be "consistent"
4 participants
@tinaselenge @scholzj @ppatierno @ShubhamRwt