FrankChen021 commented on code in PR #19548:
URL: https://github.com/apache/druid/pull/19548#discussion_r3395964595
##########
server/src/main/java/org/apache/druid/segment/join/LookupJoinableFactory.java:
##########
@@ -57,9 +61,20 @@ public Optional<Joinable> build(final DataSource dataSource,
final JoinCondition
if (condition.canHashJoin()) {
final String lookupName = lookupDataSource.getLookupName();
return lookupProvider.get(lookupName)
- .map(c ->
LookupJoinable.wrap(c.getLookupExtractorFactory().get()));
+ .map(this::buildLookupJoinable);
} else {
return Optional.empty();
}
}
+
+ private LookupJoinable buildLookupJoinable(final
LookupExtractorFactoryContainer container)
+ {
+ final LookupExtractorFactory lookupExtractorFactory =
container.getLookupExtractorFactory();
+ final Optional<RetainedLookupExtractor> retainedLookupExtractor =
+ lookupExtractorFactory.acquireRetainedLookupExtractor();
Review Comment:
I agree that wiring this into `LookupJoinable.acquireReference()` would be
the wrong lifecycle, but I don't think the Cleaner-only path is reasonable for
lookup joins as-is. The current code still retains the namespace cache in
`LookupJoinableFactory` and the join path has no deterministic close for that
retained extractor, while the scheduler intentionally skips further loads once
a retired retained cache reaches the default max of 1. So a lookup-join query
can keep later namespace updates from loading until GC or the retired-cache
timeout. I'd either keep joins on the non-retained `get()` path until the
joinable/query lifecycle can own a close hook, or include that lifecycle fix in
this PR.
Reviewed 13 files total, including 12 of 22 changed files.
--
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]
