Repository navigation
[CELEBORN-2468] Add module-specific transport bootstrap configuration - #3894
Kalvin2077 wants to merge 3 commits into
Conversation
|
cc @zaynt4606 @SteNicholas @RexXiong take a look when you get the chance. |
zaynt4606
left a comment
zaynt4606
left a comment
There was a problem hiding this comment.
Thanks for extracting the configurable bootstrap support. Please clarify the identity contract and add native-SASL composition coverage as described in the inline comment before approval.
|
@zaynt4606 Thx for your review. I've clarified the identity contract in docs/security.md: clientId retains Celeborn application-ID semantics, while independent plugin principals and authentication state belong in separate per-connection state. The ordering test now exercises configured bootstraps followed by actual native SASL. I've also added invalid-credential coverage verifying that the connection closes before any subsequent client bootstrap, native authentication challenge, or business request runs. Existing identity immutability and application authorization checks remain unchanged. For context, the planned follow-up introduces ConnectionSecurityContext to hold plugin identity and authorization policy. It also changes Celeborn's RPC infrastructure to preserve the receiving connection through dispatch, including one-way calls, so request authorization can use that context. |
There was a problem hiding this comment.
🟡 Changes recommended
Conflicting default cleanup methods break dual-interface bootstrap compatibility and can interrupt resource cleanup.
1 open issue
What changed in this PR
This PR extracts configurable transport bootstraps from #3857, allowing custom authentication handshakes without modifying Celeborn’s core transport code.
Changes:
- Adds exact-module client and server bootstrap settings.
- Loads configured plugins, orders them before native bootstraps, and manages their cleanup.
- Integrates lifecycle handling across RPC, Worker, and shuffle clients, with tests and deployment guidance.
| File | Description |
|---|---|
| worker/src/test/scala/org/apache/celeborn/service/deploy/worker/WorkerBootstrapSuite.scala | Tests Worker bootstrap shutdown ordering. |
| worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala | Adds startup cleanup and closes the replication factory. |
| docs/security.md | Documents plugin deployment, ordering, and lifecycle contracts. |
| docs/configuration/network.md | Lists the new bootstrap configuration properties. |
| common/src/test/java/org/apache/celeborn/common/network/security/TransportBootstrapSuiteJ.java | Tests loading, native authentication composition, and cleanup. |
| common/src/main/scala/org/apache/celeborn/common/rpc/netty/NettyRpcEnv.scala | Cleans up resources after transport initialization failures. |
| common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala | Defines exact-module bootstrap settings. |
| common/src/main/java/org/apache/celeborn/common/network/util/TransportConf.java | Exposes configured bootstrap class lists. |
| common/src/main/java/org/apache/celeborn/common/network/TransportContext.java | Loads, orders, and closes configured bootstraps. |
| common/src/main/java/org/apache/celeborn/common/network/server/TransportServerBootstrap.java | Adds cleanup and server handler contracts. |
| common/src/main/java/org/apache/celeborn/common/network/server/TransportServer.java | Incorporates configured server bootstraps. |
| common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java | Incorporates configured client bootstraps. |
| common/src/main/java/org/apache/celeborn/common/network/client/TransportClientBootstrap.java | Adds cleanup and concurrent-use contracts. |
| client/src/test/java/org/apache/celeborn/client/ShuffleClientBootstrapSuiteJ.java | Tests lazy initialization, retries, and cleanup. |
| client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java | Defers secured initialization and cleans up failures. |
| client-flink/common/src/test/java/org/apache/celeborn/plugin/flink/FlinkBootstrapSuiteJ.java | Tests Flink context initialization and shutdown. |
| client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/client/FlinkShuffleClientImpl.java | Manages the Flink read transport context lifecycle. |
Review effort: Balanced
| @Override | ||
| default void close() throws Exception {} |
There was a problem hiding this comment.
Using the same class for both client and server bootstraps is unusual, but it is a valid implementation.
I've moved the default close() into a shared TransportBootstrap parent, so both interfaces inherit one implementation. This resolves the compatibility issue Copilot identified for plugins implementing both interfaces without overriding close().
What changes were proposed in this pull request?
Extract the configurable transport bootstrap support from #3857:
CelebornConf.TransportContext.Why are the changes needed?
Allow deployments to plug in custom transport authentication through configuration without modifying Celeborn's core transport code.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
Adds
celeborn.<module>.client.bootstrap.classesandceleborn.<module>.server.bootstrap.classes, both empty by default. Configured bootstraps run before native registration/SASL bootstraps when enabled.How was this patch tested?
Focused validation covered bootstrap loading, ordering, cleanup, and client/Worker lifecycle. 85 tests passed on JDK 8 and 9 passed with the JDK 17 runtime, using the Flink 1.18 profile. Spotless passed.