andygrove commented on PR #7016: URL: https://github.com/apache/datafusion-comet/pull/7016#issuecomment-6337161687
Thanks @mbutrovich, I agree with the first version, and it is pushed (29fca3916b). A Comet broadcast now broadcasts a `CometBroadcastValue` that holds the chunks, with the shared decode in a `@transient lazy val`, so the process-wide map, the key and the strong references to broadcast values are gone, and so is `DecodedBroadcastCache.shared`. The four readers take `chunks` from it, and `Utils.arrowBytesKryoClasses` registers it. The new suite round-trips a value that has already been decoded through Java serialization and through Kryo with `registrationRequired`, and checks that the copy arrives with no decode. `acquire` and `forget` are still there, but only as two small steps on a single `current` slot. A decode that fails still has to stop being handed out, so the tasks that come next decode it again. I don't see a way to bound the lifetime for the `AutoCloseable` version, so the holds stay. Storage memory is charged for the block's size before any decode, and as you say the decoded copy would then live until the `ContextCleaner` collects the broadcast. I'd rather keep the lifetime we can state: freed when the last task reading it completes. -- 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]
