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

Fixes a bug with dynamic allocation forcing the executor count to be 1 even when minExecutors is set to 0 #1979

Merged
merged 1 commit into from
May 17, 2024

Conversation

peter-mcclonski
Copy link
Contributor

@peter-mcclonski peter-mcclonski commented Apr 16, 2024

🛑 Important:

Please open an issue to discuss significant work before you start. We appreciate your contributions and don't want your efforts to go to waste!

For guidelines on how to contribute, please review the CONTRIBUTING.md document.

Purpose of this PR

Fixes a bug with dynamic allocation forcing the executor count to be 1 even when minExecutors is set to 0

Change Category

Indicate the type of change by marking the applicable boxes:

  • Bugfix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that could affect existing functionality)
  • Documentation update

Rationale

Resolves otherwise unexpected behavior where the value of minExecutors was not fully respected.

Checklist

Before submitting your PR, please review the following:

  • I have conducted a self-review of my own code.
  • I have updated documentation accordingly.
  • I have added tests that prove my changes are effective or that my feature works.
  • Existing unit tests pass locally with my changes.

@peter-mcclonski
Copy link
Contributor Author

Resolves #1779

@peter-mcclonski peter-mcclonski force-pushed the dynalloc-min-execs branch 6 times, most recently from d55ad63 to d240709 Compare April 19, 2024 05:00
@peter-mcclonski
Copy link
Contributor Author

@vara-bonthu some quick feedback:

  • The build-api-docs pre-commit check isn't possible to resolve in this case. It's getting flagged because there is a code difference, but the difference isn't one that would be reflectable in the api docs. Regenerating the api-docs results in no change.
  • The Chart appVersion has been updated, but the build-helm-chart and build-spark-operator checks don't seem to know how to handle it.

Honestly not sure how to resolve these issues, since it looks like the actual resolution isn't being respected by the check. Can you advise?

@vara-bonthu
Copy link
Contributor

Did you try to rebase your PR branch with master and uplift the chat version?

@peter-mcclonski
Copy link
Contributor Author

Did you try to rebase your PR branch with master and uplift the chat version?

Yup, I had tried this previously but just re-attempted for the next time the workflow is run. I don't think that will/would assist with the api-docs issue.

…tion environments.

Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

/approve

@yuchaoran2011 please review the PR

@peter-mcclonski
Copy link
Contributor Author

@vara-bonthu Any hope of getting this merged soon?

@vara-bonthu
Copy link
Contributor

Waiting for another approval from @yuchaoran2011

@vikas-saxena02
Copy link
Contributor

/approve

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vara-bonthu, vikas-saxena02

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yuchaoran2011
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label May 17, 2024
@google-oss-prow google-oss-prow bot merged commit 153537e into kubeflow:master May 17, 2024
7 checks passed
sigmarkarl pushed a commit to spotinst/spark-on-k8s-operator that referenced this pull request Aug 7, 2024
…1 even when minExecutors is set to 0 (kubeflow#1979)

Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
…1 even when minExecutors is set to 0 (kubeflow#1979)

Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants