shellfish007 commented on PR #3767: URL: https://github.com/apache/celeborn/pull/3767#issuecomment-5427042034
@SteNicholas thanks for the ping. I've added `MasterClusterInfoSuite` covering the new SSL module selection, which was the other part of that review comment: - neither module enabled → Ratis SSL off, module resolves to `rpc_service` - only `rpc_service` enabled → Ratis SSL on via the legacy fallback (unchanged behavior) - only `ratis` enabled → Ratis SSL on via the dedicated module - both enabled → `ratis` takes precedence - `ratis` enabled while `rpc_service` is explicitly disabled On the duplicated selection logic: the point is fair — `HARaftServer.configureSsl` currently repeats the ternary rather than calling `MasterClusterInfo.ratisSslModule`, so the two could drift. I've left the structure as-is for now and the new tests pin the policy, but I'm happy to consolidate it (either by calling the Scala helper from `HARaftServer`, or by dropping the helper and rewording the comment) if you have a preference — just let me know which you'd rather see. The branch has also been updated with the latest `main`. -- 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]
