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.

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

Use resize API to scale running clusters #1541

Merged
merged 23 commits into from
Aug 24, 2022
Merged

Use resize API to scale running clusters #1541

merged 23 commits into from
Aug 24, 2022

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Aug 16, 2022

Resize API has the advantage of not restarting the cluster incase of manual scaling (num_workers) / autoscale parameter change (min_workers, max_workers)

Tested using

  1. Unit tests
  2. Acceptance test
  3. Manually

@nfx nfx self-requested a review August 16, 2022 11:43
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

Please see comments inline

clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1541 (570dd81) into master (9505748) will decrease coverage by 0.09%.
The diff coverage is 71.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
- Coverage   90.17%   90.07%   -0.10%     
==========================================
  Files         132      132              
  Lines       10560    10585      +25     
==========================================
+ Hits         9522     9534      +12     
- Misses        663      669       +6     
- Partials      375      382       +7     
Impacted Files Coverage Δ
clusters/clusters_api.go 84.87% <40.00%> (-2.85%) ⬇️
clusters/resource_cluster.go 81.64% <86.36%> (-1.17%) ⬇️

Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

please also run go fmt on changed files

clusters/acceptance/clusters_api_test.go Outdated Show resolved Hide resolved
clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Show resolved Hide resolved
@shreyas-goenka
Copy link
Contributor Author

@nfx Also merge changes from #1548 onto this PR since it was necessary for this to be correct. Please ignore them here and review them in the other PR!

pipelines/resource_pipeline.go Outdated Show resolved Hide resolved
…d to waitForClusterStatus, now we no longer wait and see whether resize api can be used, only call it if the cluster is in a running state
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

logic simplification requested

clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/clusters_api.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
@shreyas-goenka
Copy link
Contributor Author

TODO: Confirm that adding omitempty for num_workers does not cause serialisability issues for single node clusters (ie num_workers = 0)

@shreyas-goenka shreyas-goenka changed the title Use resize api instead of edit when only the num_workers / autoscale … Use resize API to scale running clusters Aug 22, 2022
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
clusters/resource_cluster.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

And please use newer testing shorthands

clusters/resource_cluster_test.go Outdated Show resolved Hide resolved
clusters/resource_cluster_test.go Outdated Show resolved Hide resolved
clusters/resource_cluster_test.go Outdated Show resolved Hide resolved
clusters/resource_cluster_test.go Outdated Show resolved Hide resolved
@nfx nfx merged commit d7faf78 into master Aug 24, 2022
@nfx nfx deleted the resize-cluster branch August 24, 2022 20:47
@nfx nfx mentioned this pull request Aug 24, 2022
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants