clintropolis commented on code in PR #19843:
URL: https://github.com/apache/druid/pull/19843#discussion_r3706406239
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -1526,6 +1534,47 @@ public void drop(final DataSegment segment)
}
}
+ /**
+ * Releases the partial-load rule applied to {@code dataSegment} in response
to an unwrapped load request: the
+ * coordinator has stopped asking for parts of the segment, so the metadata
entry and the rule's bundles are unpinned.
+ * That is what a full load means under virtual storage — nothing is pinned,
each part is fetched on demand — and
+ * reclaim of the partial state on disk is left to eviction, as it is for
{@link #drop}.
+ * <p>
+ * The rule is cleared before the info file is rewritten because clearing
cannot fail, so the in-memory state and the
+ * load announcement come out right either way. A failed rewrite leaves the
info file describing the released rule,
+ * which a restart reapplies and re-announces until the coordinator's next
load request converts the segment again.
+ * <p>
+ * Callers must hold this segment's {@link #lock(DataSegment)}, which is the
external lock that
+ * {@link PartialSegmentMetadataCacheEntry#clearRule} requires to be
serialized against
+ * {@link PartialSegmentMetadataCacheEntry#applyRule}.
+ */
+ private void releaseRuleForFullLoad(DataSegment dataSegment,
PartialSegmentMetadataCacheEntry partial)
+ {
+ // Snapshot both before clearRule zeroes out the rule state so the log can
describe what was released.
+ final String priorFingerprint = partial.getRuleFingerprint();
+ final long priorRealizedBytes = partial.getRealizedBytes();
+ partial.clearRule();
+ log.info(
+ "Released partial-load rule[fingerprint=%s, realizedBytes=%d] for
segment[%s]; it is a regular full load now.",
+ priorFingerprint,
+ priorRealizedBytes,
+ dataSegment.getId()
+ );
+ try {
+ rewriteInfoFile(dataSegment);
+ }
+ catch (IOException e) {
+ log.warn(
Review Comment:
you could flip the order i think if you wanted to only release the rule
holds if you updated the info file
##########
server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java:
##########
@@ -1338,15 +1338,23 @@ public DataSegment load(final DataSegment dataSegment)
throws SegmentLoadingExce
return loadPartial(dataSegment);
}
// virtual storage doesn't do anything with loading immediately, but
check to see if the segment is already cached
- // and if so, clear out the onUnmount action
+ // and if so, clear out the onUnmount action. An unwrapped request for a
segment currently held under a
+ // partial-load rule is the coordinator asking for the whole segment
again, so release the rule as well.
+ final boolean isFullLoadRequest =
!PartialLoadSpec.detectPartialLoadSpec(dataSegment.getLoadSpec());
final ReferenceCountingLock lock = lock(dataSegment);
synchronized (lock) {
try {
final SegmentCacheEntryIdentifier cacheEntryIdentifier = new
SegmentCacheEntryIdentifier(dataSegment.getId());
for (StorageLocation location : locations) {
final SegmentCacheEntry cacheEntry =
location.getCacheEntry(cacheEntryIdentifier);
- if (cacheEntry != null) {
- cacheEntry.setOnUnmount(null);
+ if (cacheEntry == null) {
+ continue;
+ }
+ cacheEntry.setOnUnmount(null);
+ if (isFullLoadRequest
Review Comment:
do you need to check this here? if this is true, we already should have
called loadPartial instead
--
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]
