cloud-fan commented on code in PR #57828:
URL: https://github.com/apache/spark/pull/57828#discussion_r3737904044
##########
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 =>
Review Comment:
Fixed in e222f602e65. Both isolation catches now use `NonFatal`, so
`InterruptedException` and other fatal/control-flow exceptions continue to
propagate.
##########
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:
Documented this startup-versus-renewal retry asymmetry in the PR description.
##########
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:
Fixed in e222f602e65. The duplicated branches are now a single `try` with a
guarded `case NonFatal(e) if isolateFailures`, preserving propagation on the
Kerberos path.
##########
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:
Fixed in e222f602e65. Provider loading still uses the standard per-service
enable key, while failure injection now uses the dedicated
`spark.test.requirement-failing.throw` key.
--
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]
