[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...

2017-12-13 Thread vanzin 
Github user vanzin commented on the issue:

https://github.com/apache/spark/pull/19973
  
Actually... is this right? The `defaultValueString` will now be the default 
value of the parent conf, not its current value...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...

2017-12-13 Thread vanzin 
Github user vanzin commented on the issue:

https://github.com/apache/spark/pull/19973
  
LGTM. Can you say `FallbackConfigEntry` in the title instead?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...

2017-12-13 Thread SparkQA 
Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/19973
  
**[Test build #84882 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84882/testReport)**
 for PR 19973 at commit 
[`385c300`](https://github.com/apache/spark/commit/385c300c14a382654c2a1f94ccd2813487dbe26a).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...

2017-12-13 Thread rxin 
Github user rxin commented on the issue:

https://github.com/apache/spark/pull/19973
  
The issue is in

```
  /**
   * Return the `string` value of Spark SQL configuration property for the 
given key. If the key is
   * not set yet, return `defaultValue`.
   */
  def getConfString(key: String, defaultValue: String): String = {
if (defaultValue != null && defaultValue != "") {
  val entry = sqlConfEntries.get(key)
  if (entry != null) {
// Only verify configs in the SQLConf object
entry.valueConverter(defaultValue)
  }
}
Option(settings.get(key)).getOrElse(defaultValue)
  }
```

The value converter gets applied on this generated string which is not a 
real value and will fail.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark issue #19973: [SPARK-22779] ConfigEntry's default value should actuall...

2017-12-13 Thread rxin 
Github user rxin commented on the issue:

https://github.com/apache/spark/pull/19973
  
cc @vanzin @gatorsmile 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org