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

[SPARK-29148][CORE][FOLLOW-UP] Don't dynamic allocation warning when it's disabled #27615

Closed
wants to merge 1 commit into from

Conversation

HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

Currently, after #27313, it shows the warning about dynamic allocation which is disabled by default.

$ ./bin/spark-shell
...
20/02/18 11:04:56 WARN ResourceProfile: Please ensure that the number of slots available on your executors is 
limited by the number of cores to task cpus and not another custom resource. If cores is not the limiting resource 
then dynamic allocation will not work properly!

This PR brings back the configuration checking for this warning. Seems mistakenly removed at https://github.com/apache/spark/pull/27313/files#diff-364713d7776956cb8b0a771e9b62f82dL2841

Why are the changes needed?

To remove false warning.

Does this PR introduce any user-facing change?

Yes, it will don't show the warning. It's master only change so no user-facing to end users.

How was this patch tested?

Manually tested.

@HyukjinKwon
Copy link
Member Author

@tgravescs mind taking a look please?

@SparkQA
Copy link

SparkQA commented Feb 18, 2020

Test build #118609 has finished for PR 27615 at commit abf69d0.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@Ngone51
Copy link
Member

Ngone51 commented Feb 18, 2020

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Feb 18, 2020

Test build #118627 has finished for PR 27615 at commit abf69d0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master.
Since this is a warning, I verified this manually, too.
Thank you, @HyukjinKwon and @Ngone51 .

@HyukjinKwon
Copy link
Member Author

Thanks all!

@tgravescs
Copy link
Contributor

sorry was out on vacation, actually I found that this is affected by more then dynamic allocation which is why it was there all the time. The core scheduler code with barrier scheduling for instance relies on slots, there is other code sprinkled throughout that looks at "slots" that assumes cpus are limiting factor. So really I'm more for either doing more checking if custom resource specified or put the warning back with clarification.

@tgravescs
Copy link
Contributor

yeah so looked at it a bit more the check should only warn when custom resources were specified and it should be there all the time and remove the sentence about dynamic allocation. I'll put up a pr once jira comes back up

@tgravescs
Copy link
Contributor

@HyukjinKwon
Copy link
Member Author

Thanks @tgravescs.

@HyukjinKwon HyukjinKwon deleted the SPARK-29148 branch March 3, 2020 01:16
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
…it's disabled

### What changes were proposed in this pull request?

Currently, after apache#27313, it shows the warning about dynamic allocation which is disabled by default.

```bash
$ ./bin/spark-shell
```

```
...
20/02/18 11:04:56 WARN ResourceProfile: Please ensure that the number of slots available on your executors is
limited by the number of cores to task cpus and not another custom resource. If cores is not the limiting resource
then dynamic allocation will not work properly!
```

This PR brings back the configuration checking for this warning. Seems mistakenly removed at https://github.com/apache/spark/pull/27313/files#diff-364713d7776956cb8b0a771e9b62f82dL2841

### Why are the changes needed?

To remove false warning.

### Does this PR introduce any user-facing change?

Yes, it will don't show the warning. It's master only change so no user-facing to end users.

### How was this patch tested?

Manually tested.

Closes apache#27615 from HyukjinKwon/SPARK-29148.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
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.

5 participants