You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/05 20:08:38 UTC
[GitHub] [spark] steveloughran opened a new pull request, #38084: SPARK-40640. SparkHadoopUtil to set origin of hadoop/hive config options
steveloughran opened a new pull request, #38084:
URL: https://github.com/apache/spark/pull/38084
### What changes were proposed in this pull request?
The options passed from spark conf, hive-site.xml, AWS env vars now all record this in their source attribute of the entries.
The Configuration Writable methods do not propagate this, so it is not as useful cluster-wide than it could be. It does help with some of the basic troubleshooting.
### Why are the changes needed?
Helps when troubleshooting where options make their way down. These can be examined
and logged later.
For example, my cloudstore diagnosticss JAR can do this in its storediag command
and in an s3a AWS credential provider. I may add some of that logging
at debug to the ASF hadoop implementations.
https://github.com/steveloughran/cloudstore
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Not *really*. It's a very low level diagnostics feature in the Hadoop configuration classes.
### How was this patch tested?
New tests added; existing tests enhanced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989980574
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -438,26 +440,48 @@ private[spark] object SparkHadoopUtil extends Logging {
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
// Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ val env: util.Map[String, String] = System.getenv
+ appendS3CredentialsFromEnvironment(hadoopConf, env)
Review Comment:
i want it isolated for testing; we can't set env vars in a unit test, but we can instead build up a map and verify propagation there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
attilapiros commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1270712621
Interesting locally (on its own) it was passing:
```
➜ spark-pr1 git:(pr/38084) ✗ git log --oneline -n 1
7909817173 (HEAD -> pr/38084) SPARK-40640. SparkHadoopUtil to set origin of hadoop/hive config options
➜ spark-pr1 git:(pr/38084) ✗ ./build/sbt -Pyarn 'project yarn; testOnly *.YarnClusterSuite -- -z "yarn-cluster should respect conf overrides in SparkHadoopUtil"'
....
[info] compiling 4 Scala sources to /Users/attilazsoltpiros/git/attilapiros/spark-pr1/resource-managers/yarn/target/scala-2.12/test-classes ...
[info] YarnClusterSuite:
[info] - yarn-cluster should respect conf overrides in SparkHadoopUtil (SPARK-16414, SPARK-23630) (13 seconds, 231 milliseconds)
[info] Run completed in 34 seconds, 440 milliseconds.
[info] Total number of tests run: 1
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 59 s, completed Oct 6, 2022 2:15:07 PM
sbt:spark-yarn>
[info] shutting down sbt server
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989981209
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
Review Comment:
will need to tweak my scala ide rules, i see
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989711342
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -438,26 +440,48 @@ private[spark] object SparkHadoopUtil extends Logging {
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
// Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ val env: util.Map[String, String] = System.getenv
+ appendS3CredentialsFromEnvironment(hadoopConf, env)
appendHiveConfigs(hadoopConf)
appendSparkHadoopConfigs(conf, hadoopConf)
appendSparkHiveConfigs(conf, hadoopConf)
val bufferSize = conf.get(BUFFER_SIZE).toString
- hadoopConf.set("io.file.buffer.size", bufferSize)
+ hadoopConf.set("io.file.buffer.size", bufferSize, BUFFER_SIZE.key)
+ }
+ }
+
+ /**
+ * Append any AWS secrets from the environment variables
+ * if both `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` are set.
+ * If these two are set and `AWS_SESSION_TOKEN` is also set,
+ * then `fs.s3a.session.token`.
+ * The option is set with a source string which includes the hostname
+ * on which it was set. This can help debug propagation issues.
+ * @param hadoopConf configuration to patch
+ * @param env environment.
+ */
+ // Exposed for testing
+ private[deploy] def appendS3CredentialsFromEnvironment(
+ hadoopConf: Configuration,
+ env: util.Map[String, String]): Unit = {
Review Comment:
```scala
- hadoopConf: Configuration,
- env: util.Map[String, String]): Unit = {
+ hadoopConf: Configuration,
+ env: JMap[String, String]): Unit = {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989713735
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +498,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, "Set by Spark from hive-site.xml")
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ "Set by Spark from keys starting with 'spark.hadoop'")
}
+ val setBySpark = "Set by Spark to default values"
Review Comment:
Could you define these constants in `object SparkHadoopUtil`, please?
- `"Set by Spark from hive-site.xml"`
- `"Set by Spark from keys starting with 'spark.hive'"`
- `"Set by Spark from keys starting with 'spark.hadoop'"`
- `"Set by Spark to default values"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990383278
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +553,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ SOURCE_SPARK_HIVE)
+ }
+ }
+
+ /**
+ * Return a hostname without throwing an exception if the system
+ * does not know its own name.
Review Comment:
I'm wondering when does this happen? Apache Spark has the following codes.
```
core/src/main/scala/org/apache/spark/util/Utils.scala: val address = InetAddress.getLocalHost
core/src/main/scala/org/apache/spark/util/Utils.scala: logWarning("Your hostname, " + InetAddress.getLocalHost.getHostName + " resolves to" +
core/src/main/scala/org/apache/spark/util/Utils.scala: logWarning("Your hostname, " + InetAddress.getLocalHost.getHostName + " resolves to" +
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989715672
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,6 +89,43 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set("spark.hive.hiveoption", "value")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ // the endpoint value will not have been set
+ assertConfigMatches(hadoopConf, "hive.hiveoption", "value", hivePropagation)
+ }
+
+ /**
+ * The explicit buffer size propagation records this.
+ */
+ test("buffer size propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set(BUFFER_SIZE.key, "123")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ // the endpoint value will not have been set
+ assertConfigMatches(hadoopConf, "io.file.buffer.size", "123", BUFFER_SIZE.key)
+ }
+
+ test("aws credentials from environment variables") {
Review Comment:
```scala
- test("aws credentials from environment variables") {
+ test("SPARK-40640: aws credentials from environment variables") {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989716176
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
Review Comment:
Indentation: Two more spaces.
```scala
private def assertConfigMatches(
hadoopConf: Configuration,
key: String,
expected: String,
expectedSource: String): Unit = {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990388488
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1299,7 +1299,8 @@ private[hive] object HiveClientImpl extends Logging {
// 3: we set all entries in config to this hiveConf.
val confMap = (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue) ++
sparkConf.getAll.toMap ++ extraConfig).toMap
- confMap.foreach { case (k, v) => hiveConf.set(k, v) }
+ val fromSpark = "Set by Spark"
Review Comment:
Gentle ping about this leftover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990366656
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +553,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ SOURCE_SPARK_HIVE)
Review Comment:
Can we make it into a single line?
```scala
hadoopConf.set(key.substring("spark.".length), value, SOURCE_SPARK_HIVE)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
attilapiros commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989316425
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -500,14 +523,35 @@ private[spark] object SparkHadoopUtil extends Logging {
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ "Set by Spark from keys starting with 'spark.hive'")
+ }
+ }
+
+ /**
+ * Extract the origin of a configuration key, or a default value if
+ * the key is not found or its origin is not known.
+ * Note that options provided by credential providers (JCEKS stores etc)
+ * are not resolved, so values retrieved by Configuration.getPassword()
+ * may not be recorded as having an origin.
+ * @param hadoopConf hadoop configuration to examine.
+ * @param key key to look up
+ * @param defVal default value
+ * @return the origin of the current entry in the configuration, or the default.
+ */
+ def extractOrigin(hadoopConf: Configuration, key: String, defVal: String): String = {
Review Comment:
So do we know something about the orders?
1) Which one wins if one key is set from two different sources? The last one?
2) Does the last on is the first one in the `getPropertySources()` result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991210690
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -437,27 +490,53 @@ private[spark] object SparkHadoopUtil extends Logging {
// Note: this null check is around more than just access to the "conf" object to maintain
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
- // Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ appendS3CredentialsFromEnvironment(hadoopConf, System.getenv)
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991223604
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +553,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ SOURCE_SPARK_HIVE)
+ }
+ }
+
+ /**
+ * Return a hostname without throwing an exception if the system
+ * does not know its own name.
Review Comment:
inlining
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989715474
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,6 +89,43 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set("spark.hive.hiveoption", "value")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ // the endpoint value will not have been set
+ assertConfigMatches(hadoopConf, "hive.hiveoption", "value", hivePropagation)
+ }
+
+ /**
+ * The explicit buffer size propagation records this.
+ */
+ test("buffer size propagation") {
Review Comment:
```scala
- test("buffer size propagation") {
+ test("SPARK-40640: buffer size propagation") {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989717517
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1299,7 +1299,8 @@ private[hive] object HiveClientImpl extends Logging {
// 3: we set all entries in config to this hiveConf.
val confMap = (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue) ++
sparkConf.getAll.toMap ++ extraConfig).toMap
- confMap.foreach { case (k, v) => hiveConf.set(k, v) }
+ val fromSpark = "Set by Spark"
Review Comment:
Although this is tricky, let's collect this into the same place (https://github.com/apache/spark/pull/38084/files#r989713735).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1277897283
test failure seems nowhere near this code
```
[info] *** 1 TEST FAILED ***
[error] Failed: Total 3425, Failed 1, Errors 0, Passed 3424, Ignored 9, Canceled 2
[error] Failed tests:
[error] org.apache.spark.scheduler.TaskSetManagerSuite
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r992821803
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1299,7 +1299,8 @@ private[hive] object HiveClientImpl extends Logging {
// 3: we set all entries in config to this hiveConf.
val confMap = (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue) ++
sparkConf.getAll.toMap ++ extraConfig).toMap
- confMap.foreach { case (k, v) => hiveConf.set(k, v) }
+ val fromSpark = "Set by Spark"
Review Comment:
> yes, what is the solution there? Even when exported with private[spark] it wasn't picked up
Okay, let me make a PR to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991168920
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,17 +87,100 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set("spark.hive.hiveoption", "value")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "hive.hiveoption", "value",
+ SOURCE_SPARK_HIVE)
+ }
+
+ /**
+ * The explicit buffer size propagation records this.
+ */
+ test("SPARK-40640: buffer size propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set(BUFFER_SIZE.key, "123")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "io.file.buffer.size", "123", BUFFER_SIZE.key)
+ }
+
+ test("SPARK-40640: aws credentials from environment variables") {
+ val env = new java.util.HashMap[String, String]
+ env.put(ENV_VAR_AWS_ACCESS_KEY, "access-key")
+ env.put(ENV_VAR_AWS_SECRET_KEY, "secret-key")
+ env.put(ENV_VAR_AWS_SESSION_TOKEN, "session-token")
+ val hadoopConf = new Configuration(false)
+ SparkHadoopUtil.appendS3CredentialsFromEnvironment(hadoopConf, env)
+ val source = "Set by Spark on " + InetAddress.getLocalHost + " from "
+ assertConfigMatches(hadoopConf, "fs.s3a.access.key", "access-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.secret.key", "secret-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.session.token", "session-token", source)
+ }
+
+ test("SPARK-19739: S3 session token propagation requires access and secret keys") {
Review Comment:
its more now test is doing env var probing, it's verifying that it doesn't pick up the session ids. but if you don't want it in, i will cut
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990379772
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -437,27 +490,53 @@ private[spark] object SparkHadoopUtil extends Logging {
// Note: this null check is around more than just access to the "conf" object to maintain
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
- // Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ appendS3CredentialsFromEnvironment(hadoopConf, System.getenv)
Review Comment:
If you really want to hand over a map as a dependency injection, can you build a small one by reusing the existing logic only like `System.getenv("AWS_ACCESS_KEY_ID")`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990388866
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1299,7 +1299,8 @@ private[hive] object HiveClientImpl extends Logging {
// 3: we set all entries in config to this hiveConf.
val confMap = (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue) ++
sparkConf.getAll.toMap ++ extraConfig).toMap
- confMap.foreach { case (k, v) => hiveConf.set(k, v) }
+ val fromSpark = "Set by Spark"
Review Comment:
Since this is non-test code string, we should have in the same place with the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r992816546
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +559,61 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value, SOURCE_SPARK_HIVE)
+ }
+ }
+
+ /**
+ * Extract the sources of a configuration key, or a default value if
+ * the key is not found or it has no known sources.
+ * Note that options provided by credential providers (JCEKS stores etc)
+ * are not resolved, so values retrieved by Configuration.getPassword()
+ * may not be recorded as having an origin.
+ * @param hadoopConf hadoop configuration to examine.
+ * @param key key to look up
+ * @return the origin of the current entry in the configuration, or the empty string.
+ */
+ def propertySources(hadoopConf: Configuration, key: String): String = {
+ val sources = hadoopConf.getPropertySources(key)
+ if (sources != null && sources.nonEmpty) {
+ sources.mkString("," )
Review Comment:
nit.
```scala
- sources.mkString("," )
+ sources.mkString(",")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989714199
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -17,12 +17,19 @@
package org.apache.spark.deploy
+import java.net.InetAddress
+
import org.apache.hadoop.conf.Configuration
import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.BUFFER_SIZE
class SparkHadoopUtilSuite extends SparkFunSuite {
+ private val setToDefaultValues = "Set by Spark to default values"
+ private val hadoopPropagation = "Set by Spark from keys starting with 'spark.hadoop'"
+ private val hivePropagation = "Set by Spark from keys starting with 'spark.hive'"
Review Comment:
We can remove these by https://github.com/apache/spark/pull/38084/files#r989713735.
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -17,12 +17,19 @@
package org.apache.spark.deploy
+import java.net.InetAddress
+
import org.apache.hadoop.conf.Configuration
import org.apache.spark.{SparkConf, SparkFunSuite}
+import org.apache.spark.internal.config.BUFFER_SIZE
class SparkHadoopUtilSuite extends SparkFunSuite {
+ private val setToDefaultValues = "Set by Spark to default values"
+ private val hadoopPropagation = "Set by Spark from keys starting with 'spark.hadoop'"
+ private val hivePropagation = "Set by Spark from keys starting with 'spark.hive'"
Review Comment:
We can remove these string duplications by https://github.com/apache/spark/pull/38084/files#r989713735.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991217313
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -437,27 +490,53 @@ private[spark] object SparkHadoopUtil extends Logging {
// Note: this null check is around more than just access to the "conf" object to maintain
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
- // Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ appendS3CredentialsFromEnvironment(hadoopConf, System.getenv)
Review Comment:
having it pass down separate arguments; no map at all
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
URL: https://github.com/apache/spark/pull/38084
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
attilapiros commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989341069
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -500,14 +523,35 @@ private[spark] object SparkHadoopUtil extends Logging {
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ "Set by Spark from keys starting with 'spark.hive'")
+ }
+ }
+
+ /**
+ * Extract the origin of a configuration key, or a default value if
+ * the key is not found or its origin is not known.
+ * Note that options provided by credential providers (JCEKS stores etc)
+ * are not resolved, so values retrieved by Configuration.getPassword()
+ * may not be recorded as having an origin.
+ * @param hadoopConf hadoop configuration to examine.
+ * @param key key to look up
+ * @param defVal default value
+ * @return the origin of the current entry in the configuration, or the default.
+ */
+ def extractOrigin(hadoopConf: Configuration, key: String, defVal: String): String = {
Review Comment:
If we have the answer we should fix the documentation (in the Hadoop code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989712325
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -438,26 +440,48 @@ private[spark] object SparkHadoopUtil extends Logging {
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
// Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ val env: util.Map[String, String] = System.getenv
+ appendS3CredentialsFromEnvironment(hadoopConf, env)
Review Comment:
If `env` is used by only `appendS3CredentialsFromEnvironment`, shall we invoke `System.genenv` inside that method instead of here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989713735
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +498,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, "Set by Spark from hive-site.xml")
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ "Set by Spark from keys starting with 'spark.hadoop'")
}
+ val setBySpark = "Set by Spark to default values"
Review Comment:
Could you define these constants in `object SparkHadoopUtil`, please?
- `"Set by Spark from hive-site.xml"`
- `"Set by Spark from keys starting with 'spark.hive'"`
- `"Set by Spark to default values"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989713735
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +498,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, "Set by Spark from hive-site.xml")
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ "Set by Spark from keys starting with 'spark.hadoop'")
}
+ val setBySpark = "Set by Spark to default values"
Review Comment:
Could you define these constants in `object SparkHadoopUtil`, please?
- `"Set by Spark from hive-site.xml"`
- `"Set by Spark to default values"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989716176
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
Review Comment:
Indentation: Add two more spaces for parameters.
```scala
private def assertConfigMatches(
hadoopConf: Configuration,
key: String,
expected: String,
expectedSource: String): Unit = {
assertConfigValue(hadoopConf, key, expected)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990377021
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -437,27 +490,53 @@ private[spark] object SparkHadoopUtil extends Logging {
// Note: this null check is around more than just access to the "conf" object to maintain
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
- // Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ appendS3CredentialsFromEnvironment(hadoopConf, System.getenv)
Review Comment:
I'm still worrying about the chance of the regression of getting and passing a large map of strings here. There were some reports like this.
- https://github.com/apache/spark/pull/31244/files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1276067305
thanks, merged in. when i tried that xref of the constant to hive class things wouldn't resolve properly; clearly some local build issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1278087813
Thank you, @steveloughran and @attilapiros !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r992816546
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +559,61 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value, SOURCE_SPARK_HIVE)
+ }
+ }
+
+ /**
+ * Extract the sources of a configuration key, or a default value if
+ * the key is not found or it has no known sources.
+ * Note that options provided by credential providers (JCEKS stores etc)
+ * are not resolved, so values retrieved by Configuration.getPassword()
+ * may not be recorded as having an origin.
+ * @param hadoopConf hadoop configuration to examine.
+ * @param key key to look up
+ * @return the origin of the current entry in the configuration, or the empty string.
+ */
+ def propertySources(hadoopConf: Configuration, key: String): String = {
+ val sources = hadoopConf.getPropertySources(key)
+ if (sources != null && sources.nonEmpty) {
+ sources.mkString("," )
Review Comment:
nit.
```scala
- sources.mkString("," )
+ sources.mkString(",")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989716176
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
Review Comment:
Indentation: Two more spaces.
```scala
private def assertConfigMatches(
hadoopConf: Configuration,
key: String,
expected: String,
expectedSource: String): Unit = {
assertConfigValue(hadoopConf, key, expected)
```
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
Review Comment:
Indentation: Two more spaces for parameters.
```scala
private def assertConfigMatches(
hadoopConf: Configuration,
key: String,
expected: String,
expectedSource: String): Unit = {
assertConfigValue(hadoopConf, key, expected)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989711027
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -438,26 +440,48 @@ private[spark] object SparkHadoopUtil extends Logging {
// the behavior of the old implementation of this code, for backwards compatibility.
if (conf != null) {
// Explicitly check for S3 environment variables
- val keyId = System.getenv("AWS_ACCESS_KEY_ID")
- val accessKey = System.getenv("AWS_SECRET_ACCESS_KEY")
- if (keyId != null && accessKey != null) {
- hadoopConf.set("fs.s3.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3n.awsAccessKeyId", keyId)
- hadoopConf.set("fs.s3a.access.key", keyId)
- hadoopConf.set("fs.s3.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3n.awsSecretAccessKey", accessKey)
- hadoopConf.set("fs.s3a.secret.key", accessKey)
-
- val sessionToken = System.getenv("AWS_SESSION_TOKEN")
- if (sessionToken != null) {
- hadoopConf.set("fs.s3a.session.token", sessionToken)
- }
- }
+ val env: util.Map[String, String] = System.getenv
Review Comment:
```scala
- val env: util.Map[String, String] = System.getenv
+ val env: JMap[String, String] = System.getenv
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1270537274
that last change seems to break a yarn test
```
- yarn-cluster should respect conf overrides in SparkHadoopUtil (SPARK-16414, SPARK-23630) *** FAILED *** (3 minutes, 0 seconds)
[info] The code passed to eventually never returned normally. Attempted 190 times over 3.001129006083333 minutes. Last failure message: handle.getState().isFinal() was false. (BaseYarnClusterSuite.scala:209)
[info] org.scalatest.exceptions.TestFailedDueToTimeoutException:
[info] at org.scalatest.enablers.Retrying$$anon$4.tryTryAgain$2(Retrying.scala:219)
[info] at org.scalatest.enablers.Retrying$$anon$4.retry(Retrying.scala:226)
[info] at org.scalatest.concurrent.Eventually.eventually(Eventually.scala:313)
[info] at org.scalatest.concurrent.Eventually.eventually$(Eventually.scala:312)
[info] at org.scalatest.concurrent.Eventually$.eventually(Eventually.scala:457)
[info] at org.apache.spark.deploy.yarn.BaseYarnClusterSuite.runSpark(BaseYarnClusterSuite.scala:209)
[info] at org.apache.spark.deploy.yarn.YarnClusterSuite.$anonfun$new$6(YarnClusterSuite.scala:148)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
[info] at org.apache.spark.deploy.yarn.BaseYarnClusterSuite.$anonfun$test$1(BaseYarnClusterSuite.scala:77)
[info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info] at org.scalatest.Transformer.apply(Transformer.scala:22)
[info] at org.scalatest.Transformer.apply(Transformer.scala:20)
[info] at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:226)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989710916
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -18,9 +18,11 @@
package org.apache.spark.deploy
import java.io.{ByteArrayInputStream, ByteArrayOutputStream, DataInputStream, DataOutputStream, File, IOException}
+import java.net.{InetAddress, UnknownHostException}
import java.security.PrivilegedExceptionAction
import java.text.DateFormat
import java.util.{Arrays, Date, Locale}
+import java.util
Review Comment:
```scala
-import java.util.{Arrays, Date, Locale}
-import java.util
+import java.util.{Arrays, Date, Locale, Map => JMap}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989716752
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -93,4 +139,41 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assert(hadoopConf.get(key) === expected,
s"Mismatch in expected value of $key")
}
+
+ /**
+ * Assert that a hadoop configuration option has the expected value
+ * and has the expected source.
+ *
+ * @param hadoopConf configuration to query
+ * @param key key to look up
+ * @param expected expected value.
+ * @param expectedSource string required to be in the property source string
+ */
+ private def assertConfigMatches(
+ hadoopConf: Configuration,
+ key: String,
+ expected: String,
+ expectedSource: String): Unit = {
+ assertConfigValue(hadoopConf, key, expected)
+ assertConfigSourceContains(hadoopConf, key, expectedSource)
+ }
+
+ /**
+ * Assert that a source of a configuration matches a specific string.
+ * @param hadoopConf hadoop configuration
+ * @param key key to probe
+ * @param expectedSource expected source
+ */
+ private def assertConfigSourceContains(
+ hadoopConf: Configuration,
+ key: String,
+ expectedSource: String): Unit = {
Review Comment:
ditto. Indentation. Two more spaces for parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991225790
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -1299,7 +1299,8 @@ private[hive] object HiveClientImpl extends Logging {
// 3: we set all entries in config to this hiveConf.
val confMap = (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue) ++
sparkConf.getAll.toMap ++ extraConfig).toMap
- confMap.foreach { case (k, v) => hiveConf.set(k, v) }
+ val fromSpark = "Set by Spark"
Review Comment:
yes, what is the solution there? Even when exported with private[spark] it wasn't picked up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1269869321
one more idea. To help debug the origin of config files, how about we always patch the config with (if unset) some property like spark.diagnostics.config.origin to that hostname?
that way, if you ever get a config in your debugger, you can print that value and see where it originated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r989980702
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +498,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, "Set by Spark from hive-site.xml")
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ "Set by Spark from keys starting with 'spark.hadoop'")
}
+ val setBySpark = "Set by Spark to default values"
Review Comment:
happy to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990387422
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,17 +87,100 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set("spark.hive.hiveoption", "value")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "hive.hiveoption", "value",
+ SOURCE_SPARK_HIVE)
+ }
+
+ /**
+ * The explicit buffer size propagation records this.
+ */
+ test("SPARK-40640: buffer size propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set(BUFFER_SIZE.key, "123")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "io.file.buffer.size", "123", BUFFER_SIZE.key)
+ }
+
+ test("SPARK-40640: aws credentials from environment variables") {
+ val env = new java.util.HashMap[String, String]
+ env.put(ENV_VAR_AWS_ACCESS_KEY, "access-key")
+ env.put(ENV_VAR_AWS_SECRET_KEY, "secret-key")
+ env.put(ENV_VAR_AWS_SESSION_TOKEN, "session-token")
+ val hadoopConf = new Configuration(false)
+ SparkHadoopUtil.appendS3CredentialsFromEnvironment(hadoopConf, env)
+ val source = "Set by Spark on " + InetAddress.getLocalHost + " from "
+ assertConfigMatches(hadoopConf, "fs.s3a.access.key", "access-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.secret.key", "secret-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.session.token", "session-token", source)
+ }
+
+ test("SPARK-19739: S3 session token propagation requires access and secret keys") {
Review Comment:
I understand what you mean, but you should not add irrelevant test cover in this PR. You don't use `assertConfigMatches` or `assertConfigSourceContains` here. So, it looks like completely independent test case. Please proceed this as a separate test case PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990384704
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -32,9 +36,12 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
val hadoopConf = new Configuration(false)
sc.set("spark.hadoop.orc.filterPushdown", "true")
new SparkHadoopUtil().appendSparkHadoopConfigs(sc, hadoopConf)
- assertConfigValue(hadoopConf, "orc.filterPushdown", "true" )
- assertConfigValue(hadoopConf, "fs.s3a.downgrade.syncable.exceptions", "true")
- assertConfigValue(hadoopConf, "fs.s3a.endpoint", "s3.amazonaws.com")
+ assertConfigMatches(hadoopConf, "orc.filterPushdown", "true",
+ SOURCE_SPARK_HADOOP)
Review Comment:
Do we need to split the line? Otherwise, please make it as a single line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r990385167
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,17 +87,100 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
Review Comment:
Please add a test case prefix, `SPARK-40640: `.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
attilapiros commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1270726672
Those are flaky tests:
https://issues.apache.org/jira/browse/SPARK-32297
Please re-trigger the check!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
attilapiros commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1270452147
> one more idea. To help debug the origin of config files, how about we always patch the config with (if unset) some property like spark.diagnostics.config.origin to that hostname? that way, if you ever get a config in your debugger, you can print that value and see where it originated?
Ok but let's do that in separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r992817125
##########
core/src/test/scala/org/apache/spark/deploy/SparkHadoopUtilSuite.scala:
##########
@@ -80,17 +87,100 @@ class SparkHadoopUtilSuite extends SparkFunSuite {
assertConfigValue(hadoopConf, "fs.s3a.endpoint", null)
}
+ /**
+ * spark.hive.* is passed to the hadoop config as hive.*.
+ */
+ test("spark.hive propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set("spark.hive.hiveoption", "value")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "hive.hiveoption", "value",
+ SOURCE_SPARK_HIVE)
+ }
+
+ /**
+ * The explicit buffer size propagation records this.
+ */
+ test("SPARK-40640: buffer size propagation") {
+ val sc = new SparkConf()
+ val hadoopConf = new Configuration(false)
+ sc.set(BUFFER_SIZE.key, "123")
+ new SparkHadoopUtil().appendS3AndSparkHadoopHiveConfigurations(sc, hadoopConf)
+ assertConfigMatches(hadoopConf, "io.file.buffer.size", "123", BUFFER_SIZE.key)
+ }
+
+ test("SPARK-40640: aws credentials from environment variables") {
+ val env = new java.util.HashMap[String, String]
+ env.put(ENV_VAR_AWS_ACCESS_KEY, "access-key")
+ env.put(ENV_VAR_AWS_SECRET_KEY, "secret-key")
+ env.put(ENV_VAR_AWS_SESSION_TOKEN, "session-token")
+ val hadoopConf = new Configuration(false)
+ SparkHadoopUtil.appendS3CredentialsFromEnvironment(hadoopConf, env)
+ val source = "Set by Spark on " + InetAddress.getLocalHost + " from "
+ assertConfigMatches(hadoopConf, "fs.s3a.access.key", "access-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.secret.key", "secret-key", source)
+ assertConfigMatches(hadoopConf, "fs.s3a.session.token", "session-token", source)
+ }
+
+ test("SPARK-19739: S3 session token propagation requires access and secret keys") {
Review Comment:
Got it. New one looks good to me too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1278085045
Ya, I agree with you.
```
[info] - SPARK-32170: test SPECULATION_EFFICIENCY_TASK_DURATION_FACTOR for speculating tasks *** FAILED *** (106 milliseconds)
[info] manager.checkSpeculatableTasks(15000L) was true (TaskSetManagerSuite.scala:2487)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #38084:
URL: https://github.com/apache/spark/pull/38084#issuecomment-1278770026
thx. still not sufficient, but it may help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] steveloughran commented on a diff in pull request #38084: [SPARK-40640][CORE] SparkHadoopUtil to set origin of hadoop/hive config options
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #38084:
URL: https://github.com/apache/spark/pull/38084#discussion_r991222740
##########
core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala:
##########
@@ -474,40 +553,79 @@ private[spark] object SparkHadoopUtil extends Logging {
private def appendHiveConfigs(hadoopConf: Configuration): Unit = {
hiveConfKeys.foreach { kv =>
- hadoopConf.set(kv.getKey, kv.getValue)
+ hadoopConf.set(kv.getKey, kv.getValue, SOURCE_HIVE_SITE)
}
}
private def appendSparkHadoopConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hadoop.foo=bar" spark properties into conf as "foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hadoop.")) {
- hadoopConf.set(key.substring("spark.hadoop.".length), value)
+ hadoopConf.set(key.substring("spark.hadoop.".length), value,
+ SOURCE_SPARK_HADOOP)
}
+ val setBySpark = SET_TO_DEFAULT_VALUES
if (conf.getOption("spark.hadoop.mapreduce.fileoutputcommitter.algorithm.version").isEmpty) {
- hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1")
+ hadoopConf.set("mapreduce.fileoutputcommitter.algorithm.version", "1", setBySpark)
}
- // Since Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // In Hadoop 3.3.1, HADOOP-17597 starts to throw exceptions by default
+ // this has been reverted in 3.3.2 (HADOOP-17928); setting it to
+ // true here is harmless
if (conf.getOption("spark.hadoop.fs.s3a.downgrade.syncable.exceptions").isEmpty) {
- hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true")
+ hadoopConf.set("fs.s3a.downgrade.syncable.exceptions", "true", setBySpark)
}
// In Hadoop 3.3.1, AWS region handling with the default "" endpoint only works
// in EC2 deployments or when the AWS CLI is installed.
// The workaround is to set the name of the S3 endpoint explicitly,
// if not already set. See HADOOP-17771.
- // This change is harmless on older versions and compatible with
- // later Hadoop releases
if (hadoopConf.get("fs.s3a.endpoint", "").isEmpty &&
hadoopConf.get("fs.s3a.endpoint.region") == null) {
// set to US central endpoint which can also connect to buckets
// in other regions at the expense of a HEAD request during fs creation
- hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com")
+ hadoopConf.set("fs.s3a.endpoint", "s3.amazonaws.com", setBySpark)
}
}
private def appendSparkHiveConfigs(conf: SparkConf, hadoopConf: Configuration): Unit = {
// Copy any "spark.hive.foo=bar" spark properties into conf as "hive.foo=bar"
for ((key, value) <- conf.getAll if key.startsWith("spark.hive.")) {
- hadoopConf.set(key.substring("spark.".length), value)
+ hadoopConf.set(key.substring("spark.".length), value,
+ SOURCE_SPARK_HIVE)
+ }
+ }
+
+ /**
+ * Return a hostname without throwing an exception if the system
+ * does not know its own name.
Review Comment:
that is an interesting q. tracing the history from hadoop NetUtil and back, came in with https://issues.apache.org/jira/browse/HADOOP-1028 *and no explicit "we have seen this". I have seen machines with the wrong name, wrong IP, and even localhost not mapped to a 127. address. but not this. will cut and assume nicholas just added it to save on exception propagation in java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org