spark-test commented on code in PR #59977:
URL: https://github.com/apache/spark/pull/59977#discussion_r4461899828
##########
sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala:
##########
@@ -74,20 +74,22 @@ class ArtifactManager(session: SparkSession) extends
AutoCloseable with Logging
private val ivyCachePath = artifactPath.resolve(".ivy-cache")
+ private def ivyConfigReader: ConfigReader =
+ new ConfigReader(new SparkConfigProvider(session.conf.getAllAsJava))
+
private lazy val runtimeDependencyResolver = {
- val sparkConf = session.sparkContext.getConf
val ivyPath = Files.createDirectories(ivyCachePath).toString
new RuntimeDependencyResolver(
- ivySettingsPath = sparkConf.get(JAR_IVY_SETTING_PATH),
+ ivySettingsPath = JAR_IVY_SETTING_PATH.readFrom(ivyConfigReader),
Review Comment:
**Non-blocking (P2):** This resolver, and the two timeout vals below it, now
read `spark.jars.ivySettings`, `spark.jars.ivy` and the Ivy timeouts (with
`${...}` expansion) from the session configuration. The same `ArtifactManager`
also serves Spark Connect's server-side Maven resolution:
`SparkConnectAddArtifactsHandler` resolves client-sent `MAVEN_DEPENDENCY`
entries through `holder.artifactManager.resolveArtifacts(uri,
RejectRequestedRepositories, ...)` and caps these timeouts by the request
deadline. On a Connect server the session configuration is writable by the
client. That is why `SparkConnectConfigHandler` reads the redaction pattern
from the SparkConf instead.
So a client can send `ConfigRequest.Set` (or SQL `SET`) with
`spark.sql.legacy.setCommandRejectsSparkCoreConfs=false`, which is a runtime
SQL conf, followed by `spark.jars.ivySettings=''`. If the session has not
resolved an `ivy://` dependency yet, its next `MAVEN_DEPENDENCY` entry then
resolves through the built-in resolver chain (local caches, Maven Central,
spark-packages) instead of the operator's settings file. The same steps let it
select another Ivy settings file or local repository root on the driver host,
or raise both timeouts up to `Int.MaxValue` ms when the request has no
deadline. Without the flag, it can still redirect an operator value that uses
`${spark.<unregistered key>}` by setting that key. On the base branch all four
values came from the SparkContext configuration, which no Connect RPC can
write, so this path used only the operator's settings, as
`RejectRequestedRepositories` and the client-side resolution of explicit
repositories intend.
This affects every Connect server session: the server accepts
`MAVEN_DEPENDENCY` entries from any client and advertises
`serverSideMavenArtifacts.v1` unconditionally. Classic
`SparkSession.addArtifact` is not affected, because its caller already controls
the driver. For scale, SQL `ADD JAR` with an `ivy://` URI and a `repos`
parameter already lets a Connect client name repositories on the base branch,
so this breaks the server-side Maven path's own guarantee rather than a
server-wide boundary. `AddArtifactsHandlerSuite` sets Ivy settings only in the
driver SparkConf, so no test covers a Connect session value that differs from
the driver's.
**Recommended change:** Make server-side Spark Connect Maven resolution read
spark.jars.ivySettings, spark.jars.ivy and both Ivy timeouts, including any
${...} expansion, from the SparkContext configuration instead of the
client-writable session configuration. Note in the docs for these settings that
server-side Spark Connect resolution uses the server's configuration. Add Spark
Connect AddArtifacts handler tests in which a client-set session value differs
from the driver's.
**Why this works:** The server-side resolution path used by the AddArtifacts
handler gets its own lazily built resolver and timeout values sourced from the
SparkContext configuration. It reuses the session's .ivy-cache directory, and
the handler keeps capping those timeouts by the request deadline. Classic
SparkSession.addArtifact and addArtifacts keep the session-sourced resolver and
timeouts. Server-side values are read and expanded only against the
SparkContext configuration, so neither a client-written core key nor a
client-written substitution key can influence server-side resolution.
**Scope:** Keep server-side Spark Connect Maven resolution on the server's
own Ivy settings, while classic SparkSession.addArtifact keeps the new
session-level settings.
**Compatibility:** Classic SparkSession.addArtifact and addArtifacts keep
reading Ivy settings from the owning session. Server-side Spark Connect
resolution keeps its request-deadline capping, its rejection of URI-requested
repositories, and its session-local Ivy cache.
**Risks:** With this repair, individual Spark Connect sessions cannot use
their own Ivy settings for server-side resolution. As the pull request stands,
a client can do so only after disabling an internal legacy flag, and the base
branch used only the server's settings, so no supported Spark Connect behavior
is removed.
**Constraints:** Classic SparkSession.addArtifact and addArtifacts must keep
honoring session-specific Ivy settings, substitution and timeouts as this
change intends. Server-side resolution must keep using the session's own
.ivy-cache directory, keep capping its timeouts by the request deadline, and
keep rejecting repositories requested in the Ivy URI.
**Exclusions:**
- Client-side Spark Connect Ivy resolution and the routing of Ivy URIs
between client and server, which this change leaves to separate work.
- SQL ADD JAR with an ivy:// URI, which resolves on the driver with
repositories named in the URI and with settings from JVM system properties. The
pull request does not change it, and this repair leaves it as it is.
**Success:** On a Spark Connect server, a server-side Maven dependency
resolves with the driver's spark.jars.ivySettings, spark.jars.ivy and timeouts,
even after the client disables spark.sql.legacy.setCommandRejectsSparkCoreConfs
and writes different values or substitution keys into its session. Classic
SparkSession.addArtifact and addArtifacts still resolve with session-specific
Ivy settings. The documentation for these settings says that server-side Spark
Connect resolution uses the server's configuration.
**Verification:**
- **Regression:** A server-side Maven dependency sent after the client
disables spark.sql.legacy.setCommandRejectsSparkCoreConfs and sets
spark.jars.ivySettings in its session to an empty string or to a different
settings file still resolves only through the repository the driver configures.
- **Regression:** A client-set session value of spark.jars.ivy, written
after the client disables spark.sql.legacy.setCommandRejectsSparkCoreConfs,
does not change the local repository root that server-side resolution searches.
- **Regression:** When the driver's spark.jars.ivySettings uses a ${spark.*}
reference to an unregistered key, a client-set session value for that key does
not change the settings file that server-side resolution loads.
- **Regression:** Client-set session values of spark.jars.ivyConnectTimeout
and spark.jars.ivyReadTimeout do not change the timeouts used by server-side
resolution, which stay at the driver values capped by the request deadline.
- **Compatibility:** Classic SparkSession.addArtifact and addArtifacts
continue to honor session-specific Ivy settings, substitution and timeouts, and
keep downloaded jars and the Ivy cache session-local.
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
@@ -3085,6 +3087,7 @@ package object config {
.doc("Path to an Ivy settings file to customize resolution of jars
specified " +
"using spark.jars.packages or ivy:// URIs passed to
SparkSession.addArtifact instead " +
"of the built-in defaults, such as maven central. " +
+ "SparkSession.addArtifact reads this setting from the session
configuration. " +
Review Comment:
**Nit (P3):** This says `addArtifact` reads the setting from the session
configuration, but `ArtifactManager` captures `spark.jars.ivySettings`, and
likewise `spark.jars.ivy`, only once: when the lazy `runtimeDependencyResolver`
is built on the session's first `ivy://` resolution. It never re-reads them.
For example, after `spark.addArtifact("ivy://g:a:1")`,
`SparkSession.builder().config("spark.jars.ivySettings",
"/opt/mirror/ivysettings.xml").getOrCreate()` updates the session's value, and
`spark.conf.get` reports it, yet the next `ivy://` artifact still resolves with
the first settings file. The builder logs its generic "only runtime SQL
configurations will take effect" warning whether or not the value takes effect,
so nothing identifies this case.
The timeout entries do document a capture point, but theirs is different:
the timeouts are captured on the first path- or URI-based
`addArtifact`/`addArtifacts` call of any scheme. So after
`spark.addArtifact(localJar)`, a later timeout change is ignored while a later
settings change still applies. Every new test writes its values on a fresh
session before first use, so neither capture point is pinned: snapshotting at
construction or re-reading on every call would also pass.
**Verification:**
- **Behavior:** A session value of spark.jars.ivySettings or spark.jars.ivy
written after the session's artifact manager exists but before its first ivy://
resolution is used by that resolution.
- **Behavior:** A session value written after the session's first ivy://
resolution is not used by later resolutions in that session, and timeouts
captured by a first non-Ivy addArtifact call are not changed by a later session
write.
- **Inspection:** The four config entries' doc strings and the matching rows
of the configuration documentation state the same capture points.
--
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]
