FrankChen021 commented on code in PR #19903:
URL: https://github.com/apache/druid/pull/19903#discussion_r3728797644
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -99,12 +104,49 @@ public void run(DataSegment segment, SegmentActionHandler
handler)
PartialLoadProfile.forRequest(result.wrappedLoadSpec(),
result.fingerprint()),
getTieredReplicants()
);
- } else {
- // Matcher does not apply, but the rule still applies because
onCannotMatch == FULL_LOAD (FALL_THROUGH would
- // have caused appliesTo to return false, so run wouldn't be invoked).
Route through the regular full-load
- // handler.
- handler.replicateSegment(segment, getTieredReplicants());
+ return;
}
+ // Matcher does not apply, but the rule still does — FALL_THROUGH would
have made appliesTo return false, so run
+ // wouldn't have been invoked. How much of the segment to make resident is
onCannotMatch's call.
+ switch (onCannotMatch) {
+ case LOAD_ON_DEMAND -> handler.replicateSegment(segment,
getTieredReplicants());
+ case BASE_LOAD -> replicateWholly(
+ segment,
+ handler,
+ PartialBaseTableLoadSpec.wireForm(segment.getLoadSpec(),
PartialBaseTableLoadSpec.FINGERPRINT),
+ PartialBaseTableLoadSpec.FINGERPRINT
+ );
+ case FULL_LOAD -> replicateWholly(
Review Comment:
[P1] Honor FULL_LOAD for non-rangeable segments
On a backend that cannot open a range reader, such as zipped or legacy deep
storage, `loadPartial` clears any prior rule and returns the plain segment
without downloading or pinning anything. `FULL_LOAD` therefore degrades to weak
on-demand loading instead of making every bundle resident; because the plain
announcement has no requested fingerprint, the coordinator continues treating
the replica as stale and can requeue the same request each run. Add an eager
full-download-and-pin fallback for this wrapper, or reject or advertise the
unsupported state explicitly rather than promising `FULL_LOAD`.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java:
##########
@@ -47,17 +59,24 @@ public abstract class ProjectionPartialLoadMatcher
implements PartialLoadMatcher
/**
* Returns the sorted, deduped list of projection names from {@link
DataSegment#getProjections()} that this matcher
* selects. Returns an empty list when nothing matches (the segment exposes
no projections, or no configured pattern
- * intersects what the segment has).
+ * intersects what the segment has), which {@link #match} turns into a
base-table load rather than a non-match.
*/
protected abstract List<String> resolveProjectionNames(DataSegment segment);
+ /**
+ * Never returns {@code null}; see the class doc. Either the resolved
projections, or a base-table load when none of
+ * them are present on {@code segment}.
+ */
@Override
@Nullable
public MatchResult match(DataSegment segment, Map<String, Object>
baseLoadSpec)
{
final List<String> resolved = resolveProjectionNames(segment);
if (resolved.isEmpty()) {
- return null;
+ return new MatchResult(
Review Comment:
[P1] Respect onCannotMatch when projections are absent
Returning a base-table result here means `PartialLoadRule` never consults
`onCannotMatch` when no configured projection exists. Existing rules using
`onCannotMatch: fallThrough` followed by another load or drop rule now stop the
cascade and base-load the segment instead, silently changing tier placement and
retention behavior. Return `null` for an empty resolution and let the newly
added `BASE_LOAD` behavior opt into this fallback, or add a separate explicit
matcher option.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -99,12 +104,49 @@ public void run(DataSegment segment, SegmentActionHandler
handler)
PartialLoadProfile.forRequest(result.wrappedLoadSpec(),
result.fingerprint()),
getTieredReplicants()
);
- } else {
- // Matcher does not apply, but the rule still applies because
onCannotMatch == FULL_LOAD (FALL_THROUGH would
- // have caused appliesTo to return false, so run wouldn't be invoked).
Route through the regular full-load
- // handler.
- handler.replicateSegment(segment, getTieredReplicants());
+ return;
}
+ // Matcher does not apply, but the rule still does — FALL_THROUGH would
have made appliesTo return false, so run
+ // wouldn't have been invoked. How much of the segment to make resident is
onCannotMatch's call.
+ switch (onCannotMatch) {
+ case LOAD_ON_DEMAND -> handler.replicateSegment(segment,
getTieredReplicants());
Review Comment:
[P1] Reconcile partial profiles before going on-demand
When a replica currently announces a `partialBaseTable` or
`partialFullSegment` profile and the rule changes to `LOAD_ON_DEMAND`, this
enters the fingerprint-blind regular replication path.
`StrategicSegmentAssigner.updateReplicasInTier` exits as soon as the replica
count is satisfied, so it never sends the wrapper-free load request that makes
`SegmentLocalCacheManager` release the old rule holds. The old bundles
therefore remain pinned and the stale profile persists indefinitely. Reconcile
partial-to-regular transitions explicitly, or represent on-demand state with a
profile the partial reconciler can swap to.
--
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]
