Skip to content

[MINOR][CORE]spark.diskStore.subDirectories <= 0 should throw Exception#24024

Closed
lcqzte10192193 wants to merge 1 commit intoapache:masterfrom
lcqzte10192193:wid-lcq-190308
Closed

[MINOR][CORE]spark.diskStore.subDirectories <= 0 should throw Exception#24024
lcqzte10192193 wants to merge 1 commit intoapache:masterfrom
lcqzte10192193:wid-lcq-190308

Conversation

@lcqzte10192193
Copy link
Copy Markdown
Contributor

@lcqzte10192193 lcqzte10192193 commented Mar 8, 2019

What changes were proposed in this pull request?

this pr add check this spark.diskStore.subDirectories > 0.This value need to be checked before it can be used.

How was this patch tested?

N/A

Please review http://spark.apache.org/contributing.html before opening a pull request.

@lcqzte10192193 lcqzte10192193 changed the title [core]spark.diskStore.subDirectories<0 should throw Exception [core]spark.diskStore.subDirectories > 0 should throw Exception Mar 8, 2019
abelmokadem
abelmokadem reviewed Mar 8, 2019
View reviewed changes
Comment thread core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated
Copy link
Copy Markdown

@abelmokadem abelmokadem Mar 8, 2019

Choose a reason for hiding this comment

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

Maybe it is good to keep things consistent with other parts of the code? I see a lot of examples such as:

.checkValue(v => v > 0, "The value should be a positive integer.")

Copy link
Copy Markdown

@liupc liupc Mar 9, 2019

Choose a reason for hiding this comment

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

Agree with above, seems the message is a bit repeated with the description in the doc.

Copy link
Copy Markdown
Contributor Author

@lcqzte10192193 lcqzte10192193 Mar 9, 2019

Choose a reason for hiding this comment

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

thanks,I change this error message

Copy link
Copy Markdown

@liupc liupc Mar 9, 2019

Choose a reason for hiding this comment

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

@lcqzte10192193 The PR title should also be fixed, How about "Ensure the value of spark.diskStore.subDirectories to be a positive number?" or something else. And you should also put the jira number in the PR title.

Copy link
Copy Markdown
Member

@srowen srowen Mar 9, 2019

Choose a reason for hiding this comment

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

I'd put the key string in the error message, and just say "must be positive".
The title needs to be fixed here; it's the wrong way around. Add [MINOR] to the front. @lcqzte10192193

Copy link
Copy Markdown
Contributor Author

@lcqzte10192193 lcqzte10192193 Mar 12, 2019

Choose a reason for hiding this comment

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

thanks，I fixed it

@lcqzte10192193 lcqzte10192193 changed the title [core]spark.diskStore.subDirectories > 0 should throw Exception [MINOR][core]spark.diskStore.subDirectories > 0 should throw Exception Mar 11, 2019
@lcqzte10192193 lcqzte10192193 changed the title [MINOR][core]spark.diskStore.subDirectories > 0 should throw Exception [MINOR][CORE]spark.diskStore.subDirectories > 0 should throw Exception Mar 11, 2019
@lcqzte10192193 lcqzte10192193 changed the title [MINOR][CORE]spark.diskStore.subDirectories > 0 should throw Exception [MINOR][CORE]spark.diskStore.subDirectories <= 0 should throw Exception Mar 12, 2019
@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 12, 2019

Test build #4611 has finished for PR 24024 at commit 2ab405a.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 12, 2019

Test build #4614 has started for PR 24024 at commit 2ab405a.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 12, 2019

Test build #4615 has started for PR 24024 at commit 2ab405a.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 13, 2019

Test build #4618 has finished for PR 24024 at commit 2ab405a.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 14, 2019

Test build #4622 has started for PR 24024 at commit 2ab405a.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 15, 2019

Test build #4624 has started for PR 24024 at commit 2ab405a.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 16, 2019

Test build #4627 has started for PR 24024 at commit 2ab405a.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Mar 17, 2019

Test build #4632 has finished for PR 24024 at commit 2ab405a.

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

@srowen
Copy link
Copy Markdown
Member

srowen commented Mar 17, 2019

Merged to master

@srowen srowen closed this in 4132c98 Mar 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu approved these changes

@srowen srowen srowen approved these changes

@attilapiros attilapiros attilapiros approved these changes

+2 more reviewers

@liupc liupc liupc left review comments

@abelmokadem abelmokadem abelmokadem left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lcqzte10192193 @SparkQA @srowen @maropu @attilapiros @liupc @abelmokadem