spark-test commented on code in PR #59982:
URL: https://github.com/apache/spark/pull/59982#discussion_r4461794866
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala:
##########
@@ -601,6 +608,33 @@ private[sql] class SparkConnectClient(
*/
def preferredArrowChunkSize: Option[Int] =
configuration.preferredArrowChunkSize
+ private lazy val localIvySettingsPath: Option[String] =
+ new ConfigReader((key: String) => conf.getOption(key))
Review Comment:
**Non-blocking (P2):** This reads `spark.jars.ivySettings`, and each
`${...}` reference in it, through the Config `GetOption` RPC. The server
withholds any key or value that matches `spark.redaction.regex`
(`SparkConnectConfigHandler.handleGetOption`), and the client cannot tell that
apart from an unset key. Under the default regex
`(?i)secret|password|token|access[.]?key|credential`, a readable file such as
`/etc/secrets/ivysettings.xml` or `~/.credentials/ivysettings.xml` is skipped
without a log, and resolution falls back to the built-in defaults.
This matters most when the client launched the Connect server itself
(`--remote local` or `spark.api.mode=connect`). That launch forwards the
client's `spark.*` properties, so the file is readable on both sides. On
master, an eligible `ivy://` URI went to that server and resolved with the
configured settings. With this change it is resolved on the client with the
defaults instead. The configured repositories are bypassed, and resolution
fails when the artifact is reachable only through them, for example behind a
firewall or from an in-house repository. For URIs with `repos`, and for servers
without the capability, the new settings support never takes effect for such a
settings path. `DummySparkConnectService.config` does not model redaction, so
`ArtifactSuite` cannot catch this.
**Recommended change:** When the client JVM's own configuration sets
spark.jars.ivySettings (its spark.* system properties, which local and
spark.api.mode=connect launches already forward to the co-located server), take
the value from there and resolve its ${...} references from that same
configuration, so no part of the path depends on the config RPC. Otherwise keep
reading the session value through the config RPC as today. Add Spark Connect
client artifact tests where the session values are withheld but the client's
own configuration names a readable settings file, both directly and through a
${...} reference. Document the precedence and the remaining limitation in the
spark.jars.ivySettings and SparkSession.addArtifact documentation.
**Why this works:** When the client's own configuration names the settings
path, the client resolves it, including its references, without the server
disclosing anything, so redaction can no longer turn a configured, locally
readable settings file into the built-in defaults. The session value stays as
the fallback for clients without a local value, which keeps today's behavior
when nothing is set on the client.
**Scope:** Add a client-owned, unredacted source for the client-side Ivy
settings path ahead of the session value, with tests and documentation updates.
**Compatibility:** The server's redaction policy, server-side resolution
with the driver's settings, and the fallback to the built-in defaults for
missing, unreadable, or non-local settings.
**Risks:** A spark.jars.ivySettings system property in the client JVM now
takes precedence over the session value, and its ${...} references resolve
against the client's configuration instead of the session, so a remote client
whose JVM carries a different value uses it for client-side resolution. That
also gives remote clients a way to configure client-side resolution, which
overlaps the open question about remote clients. Dropping the precedence
reverts it.
**Constraints:** Do not change the server's config-RPC redaction or the
protocol. Server-side resolution keeps using the driver's own
spark.jars.ivySettings.
**Exclusions:**
- A redacted session value that the client does not also have in its own
configuration stays unusable on the client, because every config RPC operation
reports it exactly like an unset key. Document this limitation and its
workarounds, setting the property on the client or, for URIs without repos,
enabling spark.connect.session.serverSideMavenResolution.enabled, rather than
working around the server's redaction.
**Success:** When the client JVM's own configuration names a readable local
settings file, directly or through ${...} references it also sets, client-side
Ivy resolution loads that file even when the server withholds the corresponding
session values under spark.redaction.regex. With no client-local value,
client-side resolution behaves as before: it loads a readable session-provided
settings file and otherwise keeps the built-in defaults.
**Verification:**
- **Regression:** A client whose session config reads withhold
spark.jars.ivySettings, as the server does for a value matching
spark.redaction.regex, but whose own configuration names a readable settings
file resolves an Ivy artifact that is available only through that file's
repository, both when the client's configuration names the file directly and
when it names it through a ${...} reference to another client-side property
whose session value is also withheld.
- **Behavior:** With no client-local value, client-side resolution still
loads a readable session-provided settings file and falls back to the built-in
defaults when that file is missing.
- **Compatibility:** Ivy dependencies routed to the server are still sent as
server-side Maven dependencies, and the server's own settings use is unchanged.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala:
##########
@@ -601,6 +608,33 @@ private[sql] class SparkConnectClient(
*/
def preferredArrowChunkSize: Option[Int] =
configuration.preferredArrowChunkSize
+ private lazy val localIvySettingsPath: Option[String] =
Review Comment:
**Nit (P3):** Because this is a `lazy val`, the `GetOption` reads, the
`${...}` substitution, and the regular-file and readability checks run once per
client. The result, including `None`, is reused for every later client-side Ivy
resolution. If the settings file is created or mounted after the first
`addArtifact("ivy://...")`, later resolutions in that session keep using the
built-in defaults. If the file is removed later, the stale path still reaches
`MavenUtils.loadIvySettings`, whose `require(file.exists())` throws instead of
keeping the built-in defaults as described for a missing file. A later
`spark.conf.set` of a key referenced through `${...}` is ignored as well. The
routing preference next to it is re-read on every call, and the server-side
resolver checks the file on every resolution, so only this decision is frozen,
and a new session is the only way to pick up the change.
**Verification:**
- **Regression:** On one client, a client-side Ivy resolution before the
settings file exists uses the built-in defaults, and a second resolution after
the file is created resolves an artifact available only through that file's
repository.
- **Regression:** On one client, a client-side resolution after the
previously used settings file is deleted falls back to the built-in defaults
without an error.
- **Behavior:** Calls whose Ivy URIs are all routed to the server do not
perform the settings lookup, and each client-side resolution performs it afresh.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -128,10 +128,17 @@ class ArtifactManager(
*
* Currently it supports local files with extensions .jar and .class and
Apache Ivy URIs
*/
- def addArtifact(uri: URI): Unit = addArtifact(uri, serverSideMavenArtifacts
= false)
+ def addArtifact(uri: URI): Unit =
+ addArtifact(uri, serverSideMavenArtifacts = false, ivySettingsPath = None)
private[client] def addArtifact(uri: URI, serverSideMavenArtifacts:
Boolean): Unit =
- addArtifacts(Seq(uri), serverSideMavenArtifacts)
+ addArtifact(uri, serverSideMavenArtifacts, ivySettingsPath = None)
Review Comment:
**Nit (P3):** Nothing calls this overload any more. The public
`addArtifact(uri)` above and `SparkConnectClient.addArtifact` both use the
three-argument form now, the method is `private[client]`, and nothing
subclasses `ArtifactManager`. Its rewritten body only forwards `ivySettingsPath
= None`, so a future caller in this package that picks it would silently skip
the client-side Ivy settings. Unlike the two-argument `addArtifacts` forwarder,
which `ArtifactSuite` still uses, it has no remaining purpose.
**Verification:**
- **Compile:** The Spark Connect common and JVM client modules, including
their tests, compile without the removed overload.
- **Inspection:** No remaining call site uses a two-argument addArtifact
that takes a routing flag.
--
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]
