andygrove opened a new issue, #5529:
URL: https://github.com/apache/datafusion-comet/issues/5529
Following up on PR #5513 (part of #5352).
`CelebornTransportCallbackTracker.beginPush` appends a JDK dynamic proxy
implementing `TransportClientBootstrap` to the shared
`TransportClientFactory.clientBootstraps` list, so that every transport client
created afterwards is routed through `FactoryHook.invoke` and into
`installClient`. I think that gives a Comet-specific incompatibility the power
to break all Celeborn shuffle on the executor, not just Comet's.
Celeborn treats any bootstrap failure as fatal to client creation. From
`TransportClientFactory.createClient` in v0.6.0:
```java
for (TransportClientBootstrap clientBootstrap : clientBootstraps) {
clientBootstrap.doBootstrap(client);
}
} catch (Exception e) { // catch non-RuntimeExceptions too as bootstrap may
be written in Scala
...
client.close();
throw Throwables.propagate(e);
}
```
`installClient` and `installChannel` throw `IOException` in five places,
plus `ReflectiveOperationException` from the `field` helper. Since
`TransportClientBootstrap` declares only `void doBootstrap(TransportClient
client) throws RuntimeException`, a JDK proxy wraps those checked exceptions in
`UndeclaredThrowableException`, which `createClient` then propagates. So a
single unexpected shape in Celeborn's internals stops every new Celeborn
connection on that executor, including connections used by ordinary non-Comet
Celeborn shuffle writers.
The hook is also never uninstalled.
`CelebornShufflePusherFactory.releaseClient` removes the admission registry
entry but restores none of the patched fields, so the proxy stays in
`clientBootstraps` for the lifetime of the transport factory. That means the
exposure outlives Comet's tasks entirely. An executor that ran one Comet map
task early on still has the hook installed hours later when only stock Celeborn
shuffle is running.
Could `installClient` swallow its own failures and fall back to the
`getPushState` counting path rather than letting them escape `doBootstrap`?
Comet would lose the precise ownership signal for clients it could not
instrument, which seems much better than failing connection creation for
everyone. And could the hook be removed from `clientBootstraps` when the last
Comet client is released, so the blast radius does not persist past the tasks
that needed it?
--
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]
