andygrove opened a new issue, #5530: URL: https://github.com/apache/datafusion-comet/issues/5530
Following up on PR #5513 (part of #5352). The native Celeborn shuffle path now carries roughly 2,500 lines that reflect on Celeborn's private internals, and Celeborn is not a dependency of the build in any scope. `grep -rn celeborn` across every `pom.xml` returns nothing, and the only references to Celeborn types in `spark/src/main` are class-name string literals in `CelebornShufflePusherFactory.scala` and one `Class.forName` in `CelebornTransportCallbackTracker.java`. That means every test fixture is a hand-written Scala stand-in shaped to match what the production code expects. `CelebornShufflePartitionPusherSuite` defines `RecordingCelebornPushClient`, `AsyncRecordingCelebornPushClient`, `TransportRecordingCelebornPushClient`, `RecordingCelebornTransportClientFactory`, `RecordingCelebornTransportClientPool`, `RecordingCelebornTransportClient`, `RecordingCelebornTransportResponseHandler`, `RecordingCelebornTransportRequest`, `RecordingCelebornTransportCallback`, `RecordingCelebornPushState`, `RecordingCelebornInFlightTracker`, and `RecordingCelebornRetryExecutor`. Fixtures and production code come from the same mental model, so a divergence between that model and real Celeborn is structurally invisible to the suite. To be clear, I checked every reflected member against the real v0.6.0 and v0.7.0 sources and they all match today. The reflection targets in the PR are accurate. `ShuffleClientImpl.pushStates`, `pushDataRetryPool`, `cryptoHandler`, `getPushState`, `pushOrMergeData`, `computeBatchCRC`, `mapperEnd`, `cleanup`, and `getDataClientFactory` all resolve, as do `TransportClientFactory.clientBootstraps` and `connectionPool`, `ClientPool.clients` and `locks`, `TransportClient.channel` and `getHandler`, `TransportResponseHandler.outstandingPushes`, `PushState.exception` and `inFlightRequestTracker`, `InFlightRequestTracker.totalInflightReqs`, and `PushRequestInfo.callback`. The four-argument versus five-argument `mapperEnd` difference between 0.6 and 0.7 is handled correctly too. My concern is that nothing keeps them matching. A Celeborn point release that renames a private field, or that starts mutating `outstandingPushes` through one of the roughly thirty `ConcurrentHashMap` methods `CallbackTrackingRequests` does not override, breaks this at runtime with no compile error and no failing test. `CallbackTrackingRequests` extends `ConcurrentHashMap` but delegates to a separate backing map, so anything it does not override sees an empty map. It happens to override everything `TransportResponseHandler` uses in 0.6.0, which is why it works, but that is a coincidence rather than a contract. Would you consider adding a `provided`-scope Celeborn dependency plus one CI job that runs these suites against the real classes for 0.6 and 0.7? Even a reflection-only smoke test that asserts every field name and type resolves would convert a silent runtime break into a build failure, which seems worth a lot here given how much behavior depends on those names. -- 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]
