cecemei commented on code in PR #17943:
URL: https://github.com/apache/druid/pull/17943#discussion_r2059299093
##########
processing/src/main/java/org/apache/druid/segment/CompleteSegment.java:
##########
@@ -39,6 +40,9 @@ public class CompleteSegment implements Closeable
public CompleteSegment(@Nullable DataSegment dataSegment, Segment segment)
{
this.dataSegment = dataSegment;
+ if (segment instanceof SegmentReference) {
Review Comment:
I looked into this a bit more. The `closable` aspect of
`ReferenceCountingSegment` is all in `ReferenceCountingCloseableObject`. The
close of its `baseObject` is protected by `Phaser`, meaning it would only be
closed if there's no registered party (whoever called `Phaser.register` from
`ReferenceCountingSegment.acquireReferences`, but has not closed yet).
There're several places I see `CompleteSegment` is used. One is in
`DartDataSegmentProvider`, which wraps `CompleteSegment` and a closer (closes
the reference acquired from `ReferenceCountingSegment`) in
`ReferenceCountingResourceHolder`. So the `CompleteSegment` was never closed
directly, and its baseSegment would never be closed, since it's protected by
the mechanism above.
Another is `TaskDataSegmentProvider`, which also wraps `CompleteSegment` and
a closer (closes a `QueryableIndex` since it's loading index in
`fetchSegmentInternal`. Besides those, inline/lookup/external
`InputSliceReader` also wraps a segment with `CompleteSegment`, those seems
less relevant.
In theory when querying we don't need the `ReferenceCountingSegment` other
than knowing `getNumReferences`. Rather I think `ReferenceCountingSegment`
should have its own `close` method, meaning sometimes even though we still have
this segment, but we're not accepting new connections. But maybe we do it
elsewhere?
I guess this `SegmentReference` check is optional, but rather just nice to
know that the `segment` here is base-segment, which I feel we should not let
`SegmentReference` extends `Segment`.
--
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]
