clintropolis commented on code in PR #19535:
URL: https://github.com/apache/druid/pull/19535#discussion_r3415516060
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -526,6 +548,345 @@ public AcquireSegmentAction acquireSegment(final
DataSegment dataSegment)
}
}
+ @Override
+ public Optional<Segment> acquireCachedPartialSegment(SegmentId segmentId)
+ {
+ if (!config.isVirtualStoragePartialDownloadsEnabled()) {
+ return acquireCachedSegment(segmentId);
+ }
+ return acquireCachedInternal(segmentId, false);
+ }
+
+ @Override
+ public AcquireSegmentAction acquirePartialSegment(DataSegment dataSegment)
+ {
+ final SegmentRangeReader rangeReader = tryOpenRangeReader(dataSegment);
+ if (rangeReader == null) {
+ // Storage backend doesn't support range reads for this segment (e.g. V9
format or zipped)
+ return acquireSegment(dataSegment);
+ }
+ return acquirePartialInternal(dataSegment, rangeReader, false);
+ }
+
+
+ /**
+ * Shared implementation for {@link #acquireCachedSegment} and {@link
#acquireCachedPartialSegment}. Walks the
+ * storage locations checking for existing entries.
+ * <p>
+ * When {@code requireFullyDownloaded} is {@code true} the entry must also
report
+ * {@link SegmentCacheEntry#isFullyDownloaded()} (the contract for {@link
#acquireCachedSegment}), which never
+ * hands back an entry that isn't behaviorally fully-materialized. When
{@code false} a mounted entry is
+ * sufficient ({@link #acquireCachedPartialSegment}'s lazy-mount contract),
even if some bundles are still
+ * not yet on disk. In either case {@link
SegmentCacheEntry#acquireReference(Closeable)} composes the weak-entry hold
+ * into the returned segment's close lifecycle.
+ */
+ private Optional<Segment> acquireCachedInternal(SegmentId segmentId, boolean
requireFullyDownloaded)
+ {
+ final SegmentCacheEntryIdentifier id = new
SegmentCacheEntryIdentifier(segmentId);
+ for (StorageLocation location : locations) {
+ final SegmentCacheEntry staticEntry = location.getStaticCacheEntry(id);
+ if (staticEntry != null) {
+ if (staticEntry.isMounted() && (!requireFullyDownloaded ||
staticEntry.isFullyDownloaded())) {
+ return staticEntry.acquireReference();
+ }
+ return Optional.empty();
+ }
+ if (!config.isVirtualStorage()) {
+ continue;
+ }
+ final StorageLocation.ReservationHold<SegmentCacheEntry> hold =
location.addWeakReservationHoldIfExists(id);
+ if (hold != null) {
+ if (hold.getEntry().isMounted() && (!requireFullyDownloaded ||
hold.getEntry().isFullyDownloaded())) {
+ return hold.getEntry().acquireReference(hold);
+ }
+ hold.close();
+ return Optional.empty();
+ }
+ }
+ return Optional.empty();
+ }
+
+ /**
+ * Shared scaffolding for both partial acquire APIs. {@code
fullDownload=false} powers
+ * {@link #acquirePartialSegment} (lazy mount, header bytes only; bundles
mount on demand at query time);
+ * {@code fullDownload=true} powers the partial-eligible branch of {@link
#acquireSegment} (mount +
+ * {@link PartialSegmentFileMapperV10#ensureAllDownloaded} so the returned
segment is fully-materialized).
+ * <p>
+ * Fast path: {@link #findExistingPartialWithHold} locates an existing entry
across locations under a hold. If the
+ * entry is already usable (mounted, and fully-downloaded when required),
return an immediate-future action whose
+ * {@code loadCleanup} is the hold (the supplier mints fresh segments per
call, each with its own metadata
+ * reference, so no separate cleanup is needed).
+ * <p>
+ * Slow path: under the per-segment lock, {@link #findOrReservePartial}
reuses an existing not-yet-usable entry or
+ * reserves a fresh weak one, and the action submits mount (+ optional
ensureAllDownloaded) to
+ * {@link #virtualStorageLoadOnDemandExec} so callers that yield on the
future never block a processing thread on
+ * deep-storage I/O.
+ */
+ private AcquireSegmentAction acquirePartialInternal(
+ DataSegment dataSegment,
+ SegmentRangeReader rangeReader,
+ boolean fullDownload
Review Comment:
yea, this was an oversight in a refactor, it should be handled correctly now
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -526,6 +548,345 @@ public AcquireSegmentAction acquireSegment(final
DataSegment dataSegment)
}
}
+ @Override
+ public Optional<Segment> acquireCachedPartialSegment(SegmentId segmentId)
+ {
+ if (!config.isVirtualStoragePartialDownloadsEnabled()) {
+ return acquireCachedSegment(segmentId);
+ }
+ return acquireCachedInternal(segmentId, false);
+ }
+
+ @Override
+ public AcquireSegmentAction acquirePartialSegment(DataSegment dataSegment)
+ {
+ final SegmentRangeReader rangeReader = tryOpenRangeReader(dataSegment);
+ if (rangeReader == null) {
+ // Storage backend doesn't support range reads for this segment (e.g. V9
format or zipped)
+ return acquireSegment(dataSegment);
+ }
+ return acquirePartialInternal(dataSegment, rangeReader, false);
+ }
+
+
+ /**
+ * Shared implementation for {@link #acquireCachedSegment} and {@link
#acquireCachedPartialSegment}. Walks the
+ * storage locations checking for existing entries.
+ * <p>
+ * When {@code requireFullyDownloaded} is {@code true} the entry must also
report
+ * {@link SegmentCacheEntry#isFullyDownloaded()} (the contract for {@link
#acquireCachedSegment}), which never
+ * hands back an entry that isn't behaviorally fully-materialized. When
{@code false} a mounted entry is
+ * sufficient ({@link #acquireCachedPartialSegment}'s lazy-mount contract),
even if some bundles are still
+ * not yet on disk. In either case {@link
SegmentCacheEntry#acquireReference(Closeable)} composes the weak-entry hold
+ * into the returned segment's close lifecycle.
+ */
+ private Optional<Segment> acquireCachedInternal(SegmentId segmentId, boolean
requireFullyDownloaded)
+ {
+ final SegmentCacheEntryIdentifier id = new
SegmentCacheEntryIdentifier(segmentId);
+ for (StorageLocation location : locations) {
+ final SegmentCacheEntry staticEntry = location.getStaticCacheEntry(id);
+ if (staticEntry != null) {
+ if (staticEntry.isMounted() && (!requireFullyDownloaded ||
staticEntry.isFullyDownloaded())) {
+ return staticEntry.acquireReference();
+ }
+ return Optional.empty();
+ }
+ if (!config.isVirtualStorage()) {
+ continue;
+ }
+ final StorageLocation.ReservationHold<SegmentCacheEntry> hold =
location.addWeakReservationHoldIfExists(id);
+ if (hold != null) {
+ if (hold.getEntry().isMounted() && (!requireFullyDownloaded ||
hold.getEntry().isFullyDownloaded())) {
+ return hold.getEntry().acquireReference(hold);
+ }
+ hold.close();
+ return Optional.empty();
+ }
+ }
+ return Optional.empty();
+ }
+
+ /**
+ * Shared scaffolding for both partial acquire APIs. {@code
fullDownload=false} powers
+ * {@link #acquirePartialSegment} (lazy mount, header bytes only; bundles
mount on demand at query time);
+ * {@code fullDownload=true} powers the partial-eligible branch of {@link
#acquireSegment} (mount +
+ * {@link PartialSegmentFileMapperV10#ensureAllDownloaded} so the returned
segment is fully-materialized).
+ * <p>
+ * Fast path: {@link #findExistingPartialWithHold} locates an existing entry
across locations under a hold. If the
+ * entry is already usable (mounted, and fully-downloaded when required),
return an immediate-future action whose
+ * {@code loadCleanup} is the hold (the supplier mints fresh segments per
call, each with its own metadata
+ * reference, so no separate cleanup is needed).
+ * <p>
+ * Slow path: under the per-segment lock, {@link #findOrReservePartial}
reuses an existing not-yet-usable entry or
+ * reserves a fresh weak one, and the action submits mount (+ optional
ensureAllDownloaded) to
+ * {@link #virtualStorageLoadOnDemandExec} so callers that yield on the
future never block a processing thread on
+ * deep-storage I/O.
+ */
+ private AcquireSegmentAction acquirePartialInternal(
+ DataSegment dataSegment,
+ SegmentRangeReader rangeReader,
+ boolean fullDownload
Review Comment:
yea, this was an oversight in a refactor, good catch; it should be handled
correctly now
--
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]
