capistrant commented on code in PR #19903:
URL: https://github.com/apache/druid/pull/19903#discussion_r3731119428
##########
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:
I don't think we actually end up with re-queue constantly in this case. wont
we just lazily load the full segment, but still have announced it with the full
fingerprint and full size accounting?
one alternative I've thought about for handling non range readable segments
is a metric emitted with datasource, fingerprint dims when end up loading a
situation like this. I'm not sure how difficult having a full load for non
ranageable segments is in this case, so I figured a metric alerting an operator
to this could be more straightforward to implement
##########
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:
> Reconcile partial-to-regular transitions explicitly, or represent
on-demand state with a profile the partial reconciler can swap to.
The latter sounds better to me.
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/ProjectionPartialLoadMatcher.java:
##########
@@ -33,12 +34,23 @@
/**
* Base for {@link PartialLoadMatcher} implementations that decide which of a
segment's V10 projections to load.
* Subclasses supply the resolution policy via {@link
#resolveProjectionNames(DataSegment)}; this base handles
- * fingerprint computation and wraps the result into the {@code
partialProjection} load-spec wire form consumed
- * by the historical-side {@link PartialProjectionLoadSpec}.
+ * fingerprint computation and wraps the result into the {@code
partialProjection} load spec consumed by the
+ * historical-side {@link PartialProjectionLoadSpec}.
* <p>
* The fingerprint is a hash of what projections are partially loaded on a
segment by this rule; the data node will
* include this value in the segment announcement so that it can be used as a
lightweight value to compare against
* to handle things like rule change so that we can ensure that the 'right'
partial load is in place from run to run.
+ * <p>
+ * <b>Projection matchers always apply.</b> When none of the configured
projections are present on a segment, the
+ * matcher resolves to a {@link PartialBaseTableLoadSpec} (every row, no
projections) instead of going opaque. A
+ * projection is precomputation that is always recoverable from the base
table, so the base table is a correct
+ * substitute for one the segment doesn't carry, and it is strictly less data
than every bundle on the segment. This
+ * is the ordinary state of affairs mid-rollout, when a new projection is
being reindexed in and only some segments
+ * carry it yet.
Review Comment:
I think I understand the why for this behavior. queries that would use the
projection can still run on the data even if the projection doesn't exist. but
what about cases like:
* bad rules that misdefine a projection name and end up loading the base
table
* composites where maybe a few cluster groups + projection were spec'd in
the rule but with the base table you end up with essentially the whole segment
if the projection was missing
Either of those things is technically correct under documentation. But is it
rather something an operator should specify in the projection rule rather than
be boxed into it? maybe it even defaults like this so an operator has to
consciously say if there isn't projection match then I understand the rule will
do onCannotMatch behavior. and as I type this I ask - is this idea any better?
🫠
--
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]
