clintropolis commented on code in PR #17943:
URL: https://github.com/apache/druid/pull/17943#discussion_r2057686769
##########
processing/src/main/java/org/apache/druid/segment/ReferenceCountingSegment.java:
##########
@@ -79,6 +80,9 @@ protected ReferenceCountingSegment(
)
{
super(baseSegment);
+ if (baseSegment instanceof SegmentReference) {
+ throw DruidException.defensive("Cannot use a SegmentReference as
baseSegment for a ReferenceCountingSegment");
Review Comment:
Might be nice to include segment asString() output in exception message so
we know what kind of segment we tried to accidentally wrap as a reference
counter
```suggestion
throw DruidException.defensive("Cannot use a SegmentReference[%s] as
baseSegment for a ReferenceCountingSegment", baseSegment.asString());
```
Also i think maybe this constructor could be private?
##########
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:
hmm, so like a sad side effect of this is that if `segment` really is a
reference counted segment, by not having it be that here we lose out on the
[protection](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/ReferenceCountingSegment.java#L91)
it has in the event that the `SegmentReference` is closed (which means that
the segment could be dropped and no longer exist, but this `CompleteSegment`
could still be holding a stale reference.
Where was this causing problems? Maybe that place could just check for a
`SegmentReference` before trying to make another one?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/worker/DartDataSegmentProvider.java:
##########
@@ -95,7 +96,10 @@ public Supplier<ResourceHolder<CompleteSegment>>
fetchSegment(
final PhysicalSegmentInspector inspector =
segment.as(PhysicalSegmentInspector.class);
channelCounters.addFile(inspector != null ? inspector.getNumRows() :
0, 0);
});
- return new ReferenceCountingResourceHolder<>(new CompleteSegment(null,
segment), closer);
+ return new ReferenceCountingResourceHolder<>(new CompleteSegment(
+ null,
+ Objects.requireNonNull(segment.getBaseSegment())
+ ), closer);
Review Comment:
this is worth a comment about what is going and why we are stripping the
reference counting from the segment, however i'm not sure it is the ideal way
to handle this, see other comment on `CompleteSegment`
--
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]
