HwangDongJun commented on PR #12483:
URL: https://github.com/apache/gluten/pull/12483#issuecomment-5201428687
Thanks for working on this — we hit exactly the problem this PR is trying to
solve (S3A native scan hanging/misbehaving when Hadoop fs configs are set at
the session level rather than at job submission time, in our case via a
multi-tenant Kyuubi-based batch setup where different sessions use different
S3A credentials).
We ported this patch to test it against our workload and found two issues in
the current implementation that we think are worth flagging before this gets
merged:
## 1. SQLConf handling misses the `spark.hadoop.` prefixed form
The current SQLConf collection only matches keys that literally start with
`fs.s3a.` (etc.):
```scala
val fromSqlConf: Map[String, String] =
SQLConf.get.getAllConfs.filter {
case (k, _) => fsPrefixes.exists(k.startsWith)
}
```
However, `spark.hadoop.<key>` is the standard Spark convention for passing
Hadoop configuration (this PR's own "Source 1" comment even notes
"spark.hadoop. prefix is stripped by Spark" for
`sparkContext.hadoopConfiguration`). That stripping only happens once at
`SparkContext` init time for the *initial* SparkConf — it does not apply to
configs set later via `spark.conf.set("spark.hadoop.fs.s3a.access.key", ...)`
at the session level, which land in `SQLConf` as-is with the `spark.hadoop.`
prefix still attached.
Since session-level credential switching (e.g. via Kyuubi, Spark Connect, or
any shared/multi-tenant Spark service) is precisely the use case this PR
targets, and `spark.hadoop.*` is a very common way users set these values, this
silently drops a significant fraction of real-world configs. We fixed it by
additionally matching and stripping the `spark.hadoop.` prefix:
```scala
val hadoopPrefix = "spark.hadoop."
val fromSqlConf: Map[String, String] =
SQLConf.get.getAllConfs.flatMap {
case (k, v) if fsPrefixes.exists(k.startsWith(_)) =>
Some(k -> v)
case (k, v)
if k.startsWith(hadoopPrefix) &&
fsPrefixes.exists(k.stripPrefix(hadoopPrefix).startsWith(_)) =>
Some(k.stripPrefix(hadoopPrefix) -> v)
case _ => None
}
```
## 2. Runtime cache key only hashes config keys, not values
In the Runtime cache key computation, only the config *keys* are hashed, not
the values:
```scala
sortedKeys.forEach {
k =>
digest.update(k.getBytes("UTF-8"))
digest.update(0.toByte)
}
```
This means the cache key only depends on *which* config properties are set
(e.g. `fs.s3a.access.key`, `fs.s3a.secret.key`), not their actual values. Since
those property names are the same standard names regardless of which
credentials are used, any two sessions that both set S3A credentials — even
with completely different access/secret key pairs — would resolve to the same
cache key, and could end up sharing a native Runtime instance initialized with
the wrong session's credentials.
We haven't reproduced actual cross-session credential leakage in a live
multi-user run (our testing focused on functional correctness of config
propagation), but this looks like a real risk in multi-tenant setups from
reading the cache key logic, so we wanted to flag it. We addressed it on our
end by hashing both keys and values.
## Validation
With both fixes applied, we've verified in a real multi-tenant Kyuubi
deployment that per-session S3A credentials are correctly propagated to native
Velox, and that `batchscan=true` (native scan) works correctly and is
measurably faster than the JVM fallback across repeated runs with different
session credentials.
---
*This comment was written with the assistance of AI (used to help organize
and phrase the investigation notes).*
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
