clintropolis commented on code in PR #19671:
URL: https://github.com/apache/druid/pull/19671#discussion_r3555714145
##########
server/src/main/java/org/apache/druid/segment/loading/PartialSegmentMetadataCacheEntry.java:
##########
@@ -199,6 +230,326 @@ File getLocalCacheDir()
return localCacheDir;
}
+ /**
+ * The fingerprint of the partial-load rule currently applied to this entry,
or {@code null} if no rule has been
+ * applied. Set by {@link #applyRule(String, Set)} and cleared by {@link
#clearRule()}. {@link #doActualUnmount()}
+ * asserts that this field is {@code null} at unmount time, the {@code
metadataSelfHold} taken by applyRule pins
+ * the entry's phaser and prevents eviction reclaim / doActualUnmount from
firing while a rule is applied. A fresh
+ * mount therefore always starts with no rule applied.
+ */
+ @Nullable
+ public String getRuleFingerprint()
+ {
+ entryLock.lock();
+ try {
+ return ruleFingerprint;
+ }
+ finally {
+ entryLock.unlock();
+ }
+ }
+
+ /**
+ * The set of bundle names currently pinned by an applied partial-load rule,
or an empty set if no rule is applied.
+ * Return value is immutable; safe to inspect without further
synchronization.
+ */
+ public Set<String> getRuleSelectedBundleNames()
+ {
+ entryLock.lock();
+ try {
+ return ruleSelectedBundleNames;
+ }
+ finally {
+ entryLock.unlock();
+ }
+ }
+
+ /**
+ * Whether a partial-load rule is currently applied to this entry.
Equivalent to {@code getRuleFingerprint() != null}.
+ * While {@code true}, this entry holds a self-referential {@link
StorageLocation.ReservationHold} that prevents cache
+ * from evicting it, so the metadata's V10 header stays resident until
{@link #clearRule()} runs.
+ */
+ public boolean isRuleHeld()
+ {
+ entryLock.lock();
+ try {
+ return ruleFingerprint != null;
+ }
+ finally {
+ entryLock.unlock();
+ }
+ }
+
+ /**
+ * Apply (or replace) a partial-load rule on this entry. Must be called
after {@link #mount(StorageLocation)}, the
+ * entry must be registered with its {@link StorageLocation} so it can
acquire holds on itself and on its bundles.
+ * <p>
+ * <b>Concurrency contract.</b> The caller MUST serialize {@code applyRule}
and {@link #clearRule} for a given
+ * segment id through an external per-segment lock ({@code
SegmentLocalCacheManager} uses its
+ * {@code ReferenceCountingLock segmentLocks}). This method releases {@link
#entryLock} between Phase 2 (acquire
+ * holds outside the lock) and Phase 4 (release holds outside the lock); a
concurrent invocation of
+ * {@code applyRule}/{@code clearRule} on the same entry can observe
intermediate state during those windows and
+ * silently miss rule-hold installations or leak them. {@link #entryLock}
alone is insufficient because it is
+ * released across the acquire/release phases by design (holding it across
{@link StorageLocation} lock acquisition
+ * would invert the writeLock → entryLock ordering the storage layer
already uses for eviction).
+ * <p>
+ * On first application ({@code ruleFingerprint} transitions from {@code
null}), a self-referential
+ * {@link StorageLocation.ReservationHold} is taken to pin the metadata
entry in-cache. On repeat calls with the same
+ * {@code fingerprint} and matching {@code selectedBundleNames}, this is a
no-op. On a genuine rule swap, only the
+ * delta between the previous and new selection is applied: bundle holds for
names dropped from the selection are
+ * closed, and holds for names newly added are acquired for any bundle
already registered with this entry (later
+ * arrivals get their hold via {@link #registerBundle}).
+ * <p>
+ * Bundles whose {@link StorageLocation.ReservationHold} is currently
zero-refcount (never mounted, or evicted but
+ * unregistered before this call) will not appear in {@link #linkedBundles}
yet, so no hold is taken for them here;
+ * the caller is expected to drive an on-demand acquire per selected bundle
name to trigger a fresh mount, which will
+ * call {@link #registerBundle} and pick up the rule-hold at that point.
+ *
+ * @throws DruidException if the entry is not currently mounted
+ */
+ public void applyRule(String fingerprint, Set<String> selectedBundleNames)
+ {
+ Objects.requireNonNull(fingerprint, "fingerprint");
+ Objects.requireNonNull(selectedBundleNames, "selectedBundleNames");
+ final Set<String> newSelection = Set.copyOf(selectedBundleNames);
+
+ // Phase 1: snapshot under entryLock and compute the diff. Do NOT call
StorageLocation methods here — that would
+ // acquire the location's readLock while holding entryLock and could
deadlock with a concurrent writeLock holder
+ // that needs entryLock
+ final StorageLocation loc;
+ final boolean needsSelfHold;
+ final List<String> namesToRelease;
+ final List<String> namesToAcquire;
+ entryLock.lock();
+ try {
+ if (location == null) {
+ throw DruidException.defensive(
+ "applyRule on partial metadata entry[%s] requires the entry to be
mounted",
+ id
+ );
+ }
+ if (fingerprint.equals(ruleFingerprint) &&
newSelection.equals(ruleSelectedBundleNames)) {
+ return;
+ }
+ loc = location;
+ needsSelfHold = (metadataSelfHold == null);
+ namesToRelease = new ArrayList<>();
+ for (String name : ruleBundleHolds.keySet()) {
+ if (!newSelection.contains(name)) {
+ namesToRelease.add(name);
+ }
+ }
+ namesToAcquire = new ArrayList<>();
+ for (String name : newSelection) {
+ if (!ruleBundleHolds.containsKey(name) && findLinkedBundleByName(name)
!= null) {
+ namesToAcquire.add(name);
+ }
+ }
+ }
+ finally {
+ entryLock.unlock();
+ }
+
+ // Phase 2 + 3: acquire outside entryLock, then commit under entryLock.
Track every hold this call acquires in
+ // `uncommittedHolds`; when a hold's ownership transfers into
`metadataSelfHold` or `ruleBundleHolds`, it is
+ // removed from the list. Any throw between acquire and commit (or a
race-lose at commit) leaves the hold in
+ // `uncommittedHolds`, and the finally at Phase 4 releases it. This is
what makes applyRule all-or-nothing at
+ // the ReservationHold level: no leaks on partial failure, no stranded
self-hold under a stale fingerprint.
+ final List<StorageLocation.ReservationHold<?>> uncommittedHolds = new
ArrayList<>();
+ final List<StorageLocation.ReservationHold<?>> displacedHolds = new
ArrayList<>();
+ try {
+ final StorageLocation.ReservationHold<PartialSegmentMetadataCacheEntry>
newSelfHold;
+ if (needsSelfHold) {
+ newSelfHold = loc.addWeakReservationHoldIfExists(id);
+ if (newSelfHold == null) {
+ throw DruidException.defensive(
+ "Failed to acquire self-referential rule-hold on partial
metadata entry[%s]; entry is not weak-reserved",
+ id
+ );
+ }
+ uncommittedHolds.add(newSelfHold);
+ } else {
+ newSelfHold = null;
+ }
+ final Map<String,
StorageLocation.ReservationHold<PartialSegmentBundleCacheEntry>> acquired = new
HashMap<>();
+ for (String name : namesToAcquire) {
+ final StorageLocation.ReservationHold<PartialSegmentBundleCacheEntry>
h =
+ loc.addWeakReservationHoldIfExists(new
PartialSegmentBundleCacheEntryIdentifier(segmentId, name));
+ if (h != null) {
+ acquired.put(name, h);
+ uncommittedHolds.add(h);
+ }
+ }
+
+ // Phase 3: commit under entryLock. Ownership transfers are done by
removing from `uncommittedHolds` after
+ // installing into the field. Anything left in `uncommittedHolds` at the
end lost a race and gets released
+ // in Phase 4 alongside `displacedHolds` (the pre-existing holds diffed
out of ruleBundleHolds).
+ entryLock.lock();
+ try {
+ if (newSelfHold != null) {
+ if (metadataSelfHold == null) {
+ metadataSelfHold = newSelfHold;
+ uncommittedHolds.remove(newSelfHold);
+ }
+ // else: a concurrent applyRule installed a self-hold (shouldn't
happen under segmentLock, but defensive).
+ // newSelfHold stays in uncommittedHolds, gets released in Phase 4.
Review Comment:
removed
--
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]
