dongjoon-hyun commented on code in PR #57285:
URL: https://github.com/apache/spark/pull/57285#discussion_r3678212695
##########
core/src/main/scala/org/apache/spark/deploy/security/HadoopDelegationTokenManager.scala:
##########
@@ -293,14 +288,19 @@ private[spark] class HadoopDelegationTokenManager(
"credentials are available and direct credential providers are not
enabled.")
}
+ // If every direct provider failed and no tokens were obtained, throw so
that
+ // updateTokensTask() skips distributing empty credentials and schedules a
retry.
+ // This mirrors the Kerberos path, where a provider failure propagates
instead of
+ // sending empty tokens to executors.
+ if (failureCount > 0 && nextRenewal == Long.MaxValue) {
Review Comment:
This "all providers failed" test uses `nextRenewal == Long.MaxValue` as a
proxy for "no tokens
were obtained", but as noted by @peter-toth in
https://github.com/apache/spark/pull/57285#discussion_r3639196967,
`Long.MaxValue` is also the legitimate "providers succeeded but reported no
expiry" case - `HadoopDelegationTokenProvider.obtainDelegationTokens` may add
tokens and still return `None`
(the built-in `HBaseDelegationTokenProvider` does exactly this, and a direct
provider issuing non-expiring credentials would too).
Concrete failure: two direct providers; one adds a token and returns `None`,
the other throws.
Then `failureCount == 1` and `nextRenewal == Long.MaxValue`, so this throws
"All direct
credential providers failed" despite a successful partial fetch — the
obtained token is
discarded, executors never receive it, and every retry repeats the same
misjudgment.
Keying the check off the actual credentials content (as
`setupTokenManager()` already does)
removes the false positive while keeping the throw-on-total-failure behavior:
```suggestion
if (failureCount > 0 && creds.numberOfTokens() == 0 &&
creds.numberOfSecretKeys() == 0) {
```
Worth adding a test for the partial-success case too: a provider that
returns `None` from
`obtainDelegationTokens` alongside the failing one, asserting the fetched
token is still sent.
--
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]
