dongjoon-hyun commented on code in PR #58574:
URL: https://github.com/apache/spark/pull/58574#discussion_r3972283837
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -337,6 +346,12 @@ class SparkContext(config: SparkConf) extends Logging {
*/
def hadoopConfiguration: Configuration = _hadoopConfiguration
+ // The CredentialProviderLoader from the OIDC selection phase, reused by the
credential
+ // resolution phase so providers are initialized exactly once. Null if OIDC
is disabled or
Review Comment:
Nit: this comment still says "Null if OIDC is disabled or before
initialization"; it is now `None` (also in local mode).
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -300,7 +278,12 @@ private[spark] class UserCredentialManager(
log"returned null; skipping.")
} else {
credentialMap.put(scheme, credential)
- activeProviders += provider
+
+ // Fallback wiring for a provider that actually resolved a
credential. Idempotent
+ // with the selection phase (guarded by !contains); only on
initial acquisition.
+ if (applyProperties) {
+ applyResolvedProviderProperties(scheme, provider)
Review Comment:
This fallback runs inside the per-scheme `catch { case e: Exception => ...
}` below, after `credentialMap.put` and before the expiry update. If
`additionalSparkProperties()` throws (the case the selection-phase test covers
with `AnotherFakeCredentialProvider.throwOnProperties`), the credential stays
in the map but the `earliestExpiry` update is skipped, and the log says "Failed
to resolve credentials for scheme ... Skipping this provider", which is not
what happened. The renewal delay is then computed without that credential's
expiry.
The pre-PR loop isolated `additionalSparkProperties()` with its own
`NonFatal` catch. Could we wrap this call (or `applyDeclaredProperties` itself)
in its own `try`/`NonFatal` that only logs, or move it after the expiry update?
A `start()`-level test with a throwing provider would lock this in; the
existing one only covers `applyProviderProperties`.
##########
core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala:
##########
@@ -697,9 +697,18 @@ class CoarseGrainedSchedulerBackend(scheduler:
TaskSchedulerImpl, val rpcEnv: Rp
override def stop(): Unit = {
reviveThread.shutdownNow()
cleanupService.foreach(_.shutdownNow())
- stopExecutors()
- stopTokenManager()
- stopUserCredentialManager()
+ // Ensure the token and user-credential managers are always stopped, even
if stopExecutors()
+ // throws (e.g. the StopExecutors ask times out during shutdown, which
+ // KubernetesClusterSchedulerBackend.stop already anticipates). Otherwise
the
+ // UserCredentialManager renewal thread would be left running while
SparkContext.stop()
+ // closes the shared CredentialProviderLoader, causing the renewal task to
fail repeatedly
+ // against an already-closed loader.
+ try {
+ stopExecutors()
+ } finally {
+ stopTokenManager()
Review Comment:
Nit: if `stopTokenManager()` throws here, `stopUserCredentialManager()` is
skipped, which is the same shape of problem this block fixes for
`stopExecutors()`. Wrapping each of the two in `Utils.tryLogNonFatalError`
would make them independent.
--
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]
