[SPARK-21353][CORE]add checkValue in spark.internal.config about how to correctly set configurations #18555

heary-cao · 2017-07-06T13:13:12Z

What changes were proposed in this pull request?

Currently, some of the configuration parameters of spark.internal.config without adding checkValue function, which is incorrect, this PR will be add checkValue for the configuration parameters.

How was this patch tested?

existing test and add a new test case.

heary-cao · 2017-07-06T13:13:49Z

cc @cloud-fan , @srowen

jiangxb1987 · 2017-07-06T13:42:10Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Can you also check for other configs in this file and add checkValues as much as possible? Thanks!

OK,
please review it again.
thanks.

HyukjinKwon · 2017-07-06T15:43:34Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

I think we can remove 's'.

remove it.
thanks.
please review it again.

HyukjinKwon · 2017-07-06T15:43:40Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

jiangxb1987 · 2017-07-07T13:26:14Z

Please also update the PR description. Thanks!

cloud-fan · 2017-07-08T03:14:10Z

ok to test

cloud-fan · 2017-07-08T03:15:15Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

boolean conf doesn't need checkValue...

thanks.
please review it again.

cloud-fan · 2017-07-08T03:17:39Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

we should also mention that, if no unit is given, e.g. 500, it means 500mb. We also need to update configuration.md for this conf.

nvm, we should not mention that and always ask users to add unit.

cloud-fan · 2017-07-08T03:28:43Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T03:29:03Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:17:25Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

check if >= 0?

cloud-fan · 2017-07-08T04:17:37Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

check if >= 0?

It might be a bit out of place.
In org.apache.spark.ExecutorAllocationManagerSuite.scala
val conf2 = conf.clone().set("spark.dynamicAllocation.maxExecutors", "-1")
Why is this value set less than zero?
Not yet understood.

This is the same.

cloud-fan · 2017-07-08T04:17:46Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:18:32Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

should be greater than 0

cloud-fan · 2017-07-08T04:18:41Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:18:45Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:18:53Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:18:58Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

cloud-fan · 2017-07-08T04:19:03Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

SparkQA · 2017-07-08T05:23:15Z

Test build #79361 has finished for PR 18555 at commit b19b378.

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

SparkQA · 2017-07-08T06:02:03Z

Test build #79364 has finished for PR 18555 at commit 6512814.

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

jerryshao · 2017-07-08T23:43:42Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Actually Configuration like spark.dynamicAllocation.minExecutors already has check validation logics in the code where honors it (ExecutorAllocationManager). So probably checking here again makes duplication.

Also one limitation here for checking validation is that it could only verify the validity of single configuration, but for some confs like spark.dynamicAllocation.minExecutors they should be verified with other confs like spark.dynamicAllocation.minExecutors <= xxxx.initialExecutors <= xxx.maxExecutors.

jerryshao

I think we should create a JIRA for this issue, it is not so minor I think.

Also as I mentioned before there exists the checking duplication here and the code uses it.

jerryshao · 2017-07-09T00:55:44Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

IIUC, Should it here be Int.MaxValue / 1024?

sorry .
have update it.
thanks.

jerryshao · 2017-07-09T01:00:34Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Is it more accurate to use MiB instead of MB here? also please add WS like 0 MiB.

jerryshao · 2017-07-09T01:01:16Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

jerryshao · 2017-07-09T01:08:47Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Please unify this style to one for all the checks if possible, like _ > 0.

jerryshao · 2017-07-09T01:14:40Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Maybe we should let user to decide whether to bind to a privileged port. Taking an assumption here may potentially break some use cases.

@jerryshao , @HyukjinKwon
Because of the same port control at startServiceOnPort, do we need to open it here?

def startServiceOnPort[T](
startPort: Int,
startService: Int => (T, Int),
conf: SparkConf,
serviceName: String = ""): (T, Int) = {

require(startPort == 0 || (1024 <= startPort && startPort < 65536), "startPort should be between 1024 and 65535 (inclusive), or 0 for a random free port.") ...

}

Ah, do you mean the check was already there and the current change just brings it ahead to make it fail fast? If so, that's fine.

well, thanks!

gatorsmile · 2017-07-09T05:54:35Z

Please open a JIRA for this PR

jiangxb1987 · 2017-07-09T09:16:37Z

We should also check for duplicated config check value logics in the code, but I think these could be addressed in a follow-up PR.

SparkQA · 2017-07-09T15:01:33Z

Test build #79421 has finished for PR 18555 at commit 5040998.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-08-01T14:53:43Z

Test build #80117 has finished for PR 18555 at commit 92d86b2.

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

gatorsmile · 2017-08-02T05:22:58Z

Thanks! @heary-cao

cc @jiangxb1987 Could you take a look to ensure no behavior change will be caused by this PR?

jiangxb1987

LGTM, thanks for the effort @heary-cao !

heary-cao · 2017-08-06T06:22:27Z

@jiangxb1987
thanks.

SparkQA · 2017-08-11T19:51:28Z

Test build #80540 has finished for PR 18555 at commit 3135235.

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

heary-cao · 2017-08-12T11:14:10Z

cc @gatorsmile

vanzin · 2017-12-12T19:22:37Z

retest this please

SparkQA · 2017-12-12T19:34:35Z

Test build #84786 has finished for PR 18555 at commit 3135235.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-02-02T06:26:26Z

Test build #86961 has finished for PR 18555 at commit cac5c03.

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

SparkQA · 2018-02-03T08:05:01Z

Test build #87024 has finished for PR 18555 at commit c213be6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

heary-cao · 2018-02-03T10:51:38Z

retest this please

HyukjinKwon · 2018-02-04T09:35:51Z

ok to test

HyukjinKwon · 2018-02-04T09:36:03Z

retest this please

HyukjinKwon · 2018-02-04T09:48:07Z

Seems #18555 (comment) is missed.

SparkQA · 2018-02-04T13:20:36Z

Test build #87048 has finished for PR 18555 at commit c213be6.

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

…nfigurations

SparkQA · 2018-02-05T04:28:43Z

Test build #87053 has finished for PR 18555 at commit a0efb41.

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

heary-cao · 2018-02-05T11:38:58Z

cc @HyukjinKwon,@cloud-fan

HyukjinKwon · 2018-02-05T13:57:59Z

Hmm .. why not addressing #18555 (comment)? I think that comment makes sense.

heary-cao · 2018-02-06T06:06:43Z

cc @HyukjinKwon,

HyukjinKwon · 2018-02-06T07:43:19Z

Seems fine.

HyukjinKwon · 2018-06-09T08:57:41Z

ok to test

SparkQA · 2018-06-09T12:44:02Z

Test build #91602 has finished for PR 18555 at commit a0efb41.

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

jiangxb1987 reviewed Jul 6, 2017

View reviewed changes

HyukjinKwon reviewed Jul 6, 2017

View reviewed changes

heary-cao force-pushed the driver_host branch from fc4f269 to b19b378 Compare July 7, 2017 08:04

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

heary-cao force-pushed the driver_host branch from b19b378 to 6512814 Compare July 8, 2017 03:49

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

check if >= 0?

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

should be greater than 0

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

cloud-fan reviewed Jul 8, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/package.scala Outdated

Copy link

Contributor

cloud-fan Jul 8, 2017

Choose a reason for hiding this comment

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

ditto

jerryshao reviewed Jul 8, 2017

View reviewed changes

jerryshao reviewed Jul 9, 2017

View reviewed changes

heary-cao force-pushed the driver_host branch from 6512814 to 5040998 Compare July 9, 2017 14:52

heary-cao changed the title ~~[Minor]add checkValue in spark.internal.config about how to correctly set configurations~~ [SPARK-21353][CORE]add checkValue in spark.internal.config about how to correctly set configurations Jul 9, 2017

heary-cao force-pushed the driver_host branch from 5040998 to ee9ded8 Compare July 9, 2017 15:34

heary-cao force-pushed the driver_host branch from 0970a89 to 92d86b2 Compare August 1, 2017 11:47

jiangxb1987 approved these changes Aug 4, 2017

View reviewed changes

heary-cao force-pushed the driver_host branch 2 times, most recently from 46f1439 to 3135235 Compare August 11, 2017 07:51

heary-cao force-pushed the driver_host branch from 3135235 to cac5c03 Compare February 2, 2018 03:20

heary-cao force-pushed the driver_host branch from cac5c03 to c213be6 Compare February 3, 2018 05:09

add checkValue in spark.internal.config about how to correctly set co…

a0efb41

…nfigurations

heary-cao force-pushed the driver_host branch from c213be6 to a0efb41 Compare February 5, 2018 00:43

heary-cao closed this Jul 24, 2018

[SPARK-21353][CORE]add checkValue in spark.internal.config about how to correctly set configurations #18555

[SPARK-21353][CORE]add checkValue in spark.internal.config about how to correctly set configurations #18555

Uh oh!

Conversation

heary-cao commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

heary-cao commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Jul 7, 2017

Uh oh!

cloud-fan commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 8, 2017

Uh oh!

SparkQA commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heary-cao Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

heary-cao commented Jul 6, 2017 •

edited

Loading

heary-cao commented Jul 6, 2017 •

edited

Loading

heary-cao Jul 9, 2017 •

edited

Loading