sarutak commented on code in PR #58018:
URL: https://github.com/apache/spark/pull/58018#discussion_r3809518417
##########
core/src/main/java/org/apache/spark/security/CredentialProvider.java:
##########
@@ -102,6 +102,34 @@ default Duration suggestedTtl() {
return Duration.ofMinutes(15);
}
+ /**
+ * Returns additional Spark configuration properties that should be set when
this
+ * provider is active.
+ * <p>
+ * This method is called after {@link #init(Map)} and a successful
+ * {@link #resolve(UserContext, URI)} invocation. Implementations may
+ * assume that provider state is fully initialized when this is called.
+ * <p>
+ * The credential management layer applies these entries to {@code
SparkConf} after
+ * successful startup, only if the user has not already set them explicitly.
This
+ * allows provider modules to declare executor-side wiring (e.g., the Hadoop
+ * credentials provider class for a particular filesystem scheme) without
requiring
+ * core to have vendor-specific knowledge.
+ * <p>
+ * Keys must use the {@code spark.} prefix to be effective (SparkConf
convention).
+ * Keys with the {@code spark.hadoop.} prefix are propagated to executor-side
+ * Hadoop {@code Configuration} with the prefix stripped. Other {@code
spark.*}
+ * keys are applied as Spark-internal configuration.
+ * <p>
+ * The default implementation returns an empty map (no additional
properties).
+ *
+ * @return an unmodifiable map of property key-value pairs (never null)
+ * @since 4.4.0
+ */
+ default Map<String, String> additionalSparkProperties() {
Review Comment:
The `@return` already says "never null" for the Map itself, but there's no
constraint on the entries. A third-party implementation could return a
`HashMap` with null values, which would cause `SparkConf.set()` to throw NPE.
Please add:
```java
* @return an unmodifiable map of property key-value pairs (never null).
* Keys and values within the map must not be {@code null}.
```
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -222,10 +253,11 @@ private[spark] class UserCredentialManager(
* @return Tuple of (UserCredentials, earliest expiry across all service
credentials)
*/
private def resolveCredentials(
- ctx: UserContext): (UserCredentials, Option[Instant]) = {
+ ctx: UserContext): (UserCredentials, Option[Instant],
Seq[CredentialProvider]) = {
Review Comment:
Minor note: `resolveCredentials()` returns `activeProviders` that is only
consumed in `start()` and renewal discards it with `_`. This is fine for now,
but I'm considering a follow-up to separate provider selection from credential
resolution in `UserCredentialManager`, which would eliminate this asymmetry.
##########
core/src/test/scala/org/apache/spark/deploy/security/UserCredentialManagerSuite.scala:
##########
@@ -520,4 +520,65 @@ class UserCredentialManagerSuite extends SparkFunSuite {
assert(fakeProvider.getCloseCount === 1,
"stop() should close initialized providers exactly once")
}
+
+ // ========== additionalSparkProperties application ==========
Review Comment:
The three new tests cover the happy path, user-override, and null map but
the `catch NonFatal(e)` branch is untested. Please add a test where a
provider's `additionalSparkProperties()` throws a `RuntimeException`, and
verify that:
1. `start()` does not fail
2. Other providers' properties are still applied (if applicable)
--
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]
