dongjoon-hyun commented on code in PR #57828:
URL: https://github.com/apache/spark/pull/57828#discussion_r3736647447
##########
core/src/main/scala/org/apache/spark/deploy/security/HadoopDelegationTokenManager.scala:
##########
@@ -198,24 +207,30 @@ private[spark] class HadoopDelegationTokenManager(
val creds = new Credentials()
var failureCount = 0
val nextRenewal = delegationTokenProviders.values.flatMap { provider =>
- if (provider.delegationTokensRequired(sparkConf, hadoopConf)) {
- if (isolateFailures) {
- try {
+ if (isolateFailures) {
Review Comment:
Nit: after this restructuring, the "does not require a token" `logDebug` +
`None` branch is duplicated in both arms. A guarded catch keeps the behavior
identical without the duplication:
```scala
val nextRenewal = delegationTokenProviders.values.flatMap { provider =>
try {
if (provider.delegationTokensRequired(sparkConf, hadoopConf)) {
provider.obtainDelegationTokens(hadoopConf, sparkConf, creds)
} else {
logDebug(s"Service ${provider.serviceName} does not require a token." +
s" Check your configuration to see if security is disabled or not.")
None
}
} catch {
case NonFatal(e) if isolateFailures =>
logWarning(log"Failed to obtain credentials from " +
log"${MDC(LogKeys.SERVICE_NAME, provider.serviceName)}.", e)
failureCount += 1
None
}
}.foldLeft(Long.MaxValue)(math.min)
```
(using `NonFatal` per the other thread). Related minor point: with the
requirement check now inside the `try`, a `delegationTokensRequired` failure
also logs "Failed to obtain credentials from ...". If you keep the current
structure, distinguishing the wording as in `renewalEnabled` ("Failed to
determine whether credentials are required from ...") would make diagnosis
easier; with the single-catch form above, the generic message is probably fine.
##########
core/src/test/scala/org/apache/spark/deploy/security/NonKerberosCredentialsSuite.scala:
##########
@@ -79,6 +79,24 @@ private class TestFailingProvider extends
HadoopDelegationTokenProvider {
}
}
+private class TestRequirementFailingProvider extends
HadoopDelegationTokenProvider {
+ override def serviceName: String = "test-requirement-failing"
+
+ override def delegationTokensRequired(
+ sparkConf: SparkConf, hadoopConf: Configuration): Boolean = {
+ if (sparkConf.getBoolean(
+ "spark.security.credentials.test-requirement-failing.enabled", false))
{
Review Comment:
Nit: this reuses
`spark.security.credentials.test-requirement-failing.enabled`, which is also
the per-service enable key consulted by `isServiceEnabled` when loading
providers. It happens to work out (`true` → loaded and throws, `false` → not
loaded at all), but overloading one key with two meanings is easy to misread. A
dedicated key such as `spark.test.requirement-failing.throw` would be clearer.
##########
core/src/main/scala/org/apache/spark/deploy/security/HadoopDelegationTokenManager.scala:
##########
@@ -111,7 +111,16 @@ private[spark] class HadoopDelegationTokenManager(
def renewalEnabled: Boolean = {
hasKerberosCredentials ||
(sparkConf.get(DIRECT_CREDENTIAL_PROVIDERS_ENABLED) &&
-
delegationTokenProviders.values.exists(_.delegationTokensRequired(sparkConf,
hadoopConf)))
+ delegationTokenProviders.values.exists { provider =>
+ try {
+ provider.delegationTokensRequired(sparkConf, hadoopConf)
+ } catch {
+ case e: Exception =>
+ logWarning(log"Failed to determine whether credentials are
required from " +
+ log"${MDC(LogKeys.SERVICE_NAME, provider.serviceName)}.", e)
+ false
Review Comment:
One behavioral asymmetry worth noting (non-blocking): if the only active
provider throws transiently here at startup, `renewalEnabled` becomes `false`,
so the token manager never starts and there is no retry — only this warning. If
the same transient failure happened during a renewal cycle instead, it would be
counted in `failureCount` and a retry would be scheduled. This seems like an
acceptable trade-off for failure isolation, but it may be worth a brief mention
in the PR description.
--
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]
