You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2026/06/04 21:04:58 UTC
[PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
clintropolis opened a new pull request, #19557:
URL: https://github.com/apache/druid/pull/19557
### Description
This PR adds a post-processing step to `RunRules` so that partial-load matchers that resolve differently across segments of a shard group (e.g. `ClusterGroupPartialLoadMatcher` over range-partitioned segments) don't leave the broker with an incomplete `PartitionHolder`. `Rule.run` now returns a new `RuleRunResult` object that `RunRules` can use to 'do stuff', though most implementations today do `RuleRunResult.OK`. For these asymmetric partial matchers, there is a `ShardGroupFollowup` implementation of `RuleRunResult` that `RunRules` can use to check the siblings of a shard group to perform an 'empty' partial load to ensure that the group is fully available. I am unsure if there is enough of a pattern to make `RunRules` more generically handle `RuleRunResult`, i was planning to wait and see if any other use cases pop up before trying to make this handling more generic.
Doing an empty load like this seemed cheaper than trying to figure out how to allow the timeline to sometimes allow incomplete groups to appear as complete, since the empty partial load should be cheap for the target historicals.
changes:
* adds `PartialLoadMatcher.emptyMatch` (default null) as an opt-in way to do a zero-content "load" for matchers that can resolve asymmetrically.
* `Rule.run` returns a `RuleRunResult` (`OK` constant for most rules). `PartialLoadRule` returns a `ShardGroupFollowup` on a positive match, but only when numCorePartitions > 0.
* `RunRules` streams followups in a buffer keyed by (dataSource, interval, version), flushing at iteration boundaries (NEWEST_SEGMENT_FIRST groups segments contiguously by that triple). Flush dispatches emptyMatch loads to unmatched core siblings; siblings not part of the core partition group are skipped.
* adds `TimelineLookup.findChunks` which returns a defensive copy via `PartitionHolder.copyWithOnlyVisibleChunks` so iteration is safe outside the timeline lock.
* `PartialClusterGroupLoadSpec` allows an empty `clusterGroupIndices` list as the wire form of an empty match.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis closed pull request #19557: feat: post-pass shard-group reconciliation for asymmetric partial-load matchers
URL: https://github.com/apache/druid/pull/19557
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19557:
URL: https://github.com/apache/druid/pull/19557#discussion_r3365001182
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -88,23 +88,29 @@ public boolean appliesTo(DataSegment segment, DateTime referenceTimestamp)
}
@Override
- public void run(DataSegment segment, SegmentActionHandler handler)
+ public RuleRunResult run(DataSegment segment, SegmentActionHandler handler)
{
final PartialLoadMatcher.MatchResult result = matcher.match(segment, segment.getLoadSpec());
if (result != null) {
- // Matcher resolved: route through the partial-load handler. The wrappedLoadSpec map carries scheme-specific
- // data that the historical-side wrapper deserializes.
handler.replicateSegmentPartially(
segment,
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());
+ // Flag the shard group for a post-pass: if the matcher resolves asymmetrically across siblings, RunRules will
+ // dispatch emptyMatch loads to the unmatched ones so the broker's PartitionHolder.isComplete() check still
+ // treats the group as queryable. Matchers without an emptyMatch (the default) make the post-pass a no-op.
+ // Only the core partition group has an atomic-replace completeness requirement on the broker, segments with
+ // no core partitions (e.g. append-only streaming) don't need sibling reconciliation.
+ if (segment.getShardSpec().getNumCorePartitions() > 0) {
+ return new ShardGroupFollowup(segment, matcher, getTieredReplicants());
Review Comment:
will this create core partition set follow ups even when segments who are outside of the core are the only ones who matched? do we want that?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19557:
URL: https://github.com/apache/druid/pull/19557#discussion_r3365001182
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -88,23 +88,29 @@ public boolean appliesTo(DataSegment segment, DateTime referenceTimestamp)
}
@Override
- public void run(DataSegment segment, SegmentActionHandler handler)
+ public RuleRunResult run(DataSegment segment, SegmentActionHandler handler)
{
final PartialLoadMatcher.MatchResult result = matcher.match(segment, segment.getLoadSpec());
if (result != null) {
- // Matcher resolved: route through the partial-load handler. The wrappedLoadSpec map carries scheme-specific
- // data that the historical-side wrapper deserializes.
handler.replicateSegmentPartially(
segment,
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());
+ // Flag the shard group for a post-pass: if the matcher resolves asymmetrically across siblings, RunRules will
+ // dispatch emptyMatch loads to the unmatched ones so the broker's PartitionHolder.isComplete() check still
+ // treats the group as queryable. Matchers without an emptyMatch (the default) make the post-pass a no-op.
+ // Only the core partition group has an atomic-replace completeness requirement on the broker, segments with
+ // no core partitions (e.g. append-only streaming) don't need sibling reconciliation.
+ if (segment.getShardSpec().getNumCorePartitions() > 0) {
+ return new ShardGroupFollowup(segment, matcher, getTieredReplicants());
Review Comment:
will this create core partition set follow ups even for segments who are outside of the core? do we want that?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19557:
URL: https://github.com/apache/druid/pull/19557#discussion_r3364863591
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -88,23 +88,29 @@ public boolean appliesTo(DataSegment segment, DateTime referenceTimestamp)
}
@Override
- public void run(DataSegment segment, SegmentActionHandler handler)
+ public RuleRunResult run(DataSegment segment, SegmentActionHandler handler)
{
final PartialLoadMatcher.MatchResult result = matcher.match(segment, segment.getLoadSpec());
if (result != null) {
- // Matcher resolved: route through the partial-load handler. The wrappedLoadSpec map carries scheme-specific
- // data that the historical-side wrapper deserializes.
handler.replicateSegmentPartially(
segment,
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());
+ // Flag the shard group for a post-pass: if the matcher resolves asymmetrically across siblings, RunRules will
+ // dispatch emptyMatch loads to the unmatched ones so the broker's PartitionHolder.isComplete() check still
+ // treats the group as queryable. Matchers without an emptyMatch (the default) make the post-pass a no-op.
+ // Only the core partition group has an atomic-replace completeness requirement on the broker, segments with
+ // no core partitions (e.g. append-only streaming) don't need sibling reconciliation.
+ if (segment.getShardSpec().getNumCorePartitions() > 0) {
+ return new ShardGroupFollowup(segment, matcher, getTieredReplicants());
Review Comment:
I'm a bit confused about how this behaves when using`onCannotMatch == FULL_LOAD` like won't this cause empty loads of other segments that fell through to full load?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #19557:
URL: https://github.com/apache/druid/pull/19557#issuecomment-4640232928
^ closed in favor of #19565
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19557:
URL: https://github.com/apache/druid/pull/19557#discussion_r3364863591
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadRule.java:
##########
@@ -88,23 +88,29 @@ public boolean appliesTo(DataSegment segment, DateTime referenceTimestamp)
}
@Override
- public void run(DataSegment segment, SegmentActionHandler handler)
+ public RuleRunResult run(DataSegment segment, SegmentActionHandler handler)
{
final PartialLoadMatcher.MatchResult result = matcher.match(segment, segment.getLoadSpec());
if (result != null) {
- // Matcher resolved: route through the partial-load handler. The wrappedLoadSpec map carries scheme-specific
- // data that the historical-side wrapper deserializes.
handler.replicateSegmentPartially(
segment,
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());
+ // Flag the shard group for a post-pass: if the matcher resolves asymmetrically across siblings, RunRules will
+ // dispatch emptyMatch loads to the unmatched ones so the broker's PartitionHolder.isComplete() check still
+ // treats the group as queryable. Matchers without an emptyMatch (the default) make the post-pass a no-op.
+ // Only the core partition group has an atomic-replace completeness requirement on the broker, segments with
+ // no core partitions (e.g. append-only streaming) don't need sibling reconciliation.
+ if (segment.getShardSpec().getNumCorePartitions() > 0) {
+ return new ShardGroupFollowup(segment, matcher, getTieredReplicants());
Review Comment:
I'm a bit confused about how this in`onCannotMatch == FULL_LOAD` like won't this cause empty loads of other segments that fell through to full load?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: post-pass shard-group reconciliation for asymmetric partial-load matchers (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #19557:
URL: https://github.com/apache/druid/pull/19557#discussion_r3359008720
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/PartialLoadMatcher.java:
##########
@@ -50,6 +51,37 @@
@Nullable
MatchResult match(DataSegment segment, Map<String, Object> baseLoadSpec);
+ /**
+ * Returns the "empty" {@link MatchResult} this matcher produces for the given segment when {@link #match} would
+ * return {@code null}, or {@code null} if this matcher has no meaningful empty form.
+ *
+ * <p>An empty match is the matcher's way of saying "I do not match this segment's content, but I am willing to
+ * partial-load it with a zero-content load spec so the broker's shard-group completeness check still treats the
+ * group as queryable." The {@link RunRules} duty applies this in a post-pass: when at least one sibling in the
+ * same shard group (same dataSource, interval, version) had a positive match from this matcher, {@code RunRules}
+ * dispatches {@code emptyMatch} loads for the unmatched siblings. The post-pass only fires when there's a real
+ * positive match somewhere in the group.
+ *
+ * <p>Matchers that produce uniform per-segment decisions across a shard group don't need this and can leave the
+ * default null. Matchers that resolve per-segment within a shard group must implement this to opt into the post-pass.
+ *
+ * <p>The returned wire form should:
+ * <ul>
+ * <li>use the same {@code type} and structural shape as {@link #match}'s output, so the historical's
+ * {@code PartialLoadSpec} subtype can deserialize it without special-casing.</li>
+ * <li>carry a stable, deterministic fingerprint so the coordinator's reconciler doesn't churn replicas across
+ * runs when the same matcher produces the same empty result on the same segment.</li>
+ * <li>describe a load that's actually empty on the historical side (e.g. an empty list of projections or
+ * cluster-group indices). The historical-side {@code PartialLoadSpec} subtype must accept this form and
+ * perform no scheme-specific data download.</li>
+ * </ul>
+ */
+ @Nullable
+ default MatchResult emptyMatch(DataSegment segment, Map<String, Object> baseLoadSpec)
Review Comment:
## CodeQL / Useless parameter
The parameter 'segment' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/11294)
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/RuleRunResult.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.rules;
+
+import org.apache.druid.server.coordinator.duty.RunRules;
+
+/**
+ * Result of a {@link Rule#run} invocation. Rules return {@link #OK} when there is nothing for the {@link RunRules}
+ * duty to do beyond the actions the rule has already queued on the {@link SegmentActionHandler}. Rules that need
+ * follow-up coordination across siblings in a shard group return a typed implementation (today only
+ * {@link ShardGroupFollowup}); the {@link RunRules} duty dispatches on the concrete type and applies it at an
+ * appropriate point during iteration.
+ * <p>
+ * To add a new kind of post-processing, implement this interface and add a dispatch branch in {@link RunRules}.
+ */
+public interface RuleRunResult
+{
+ RuleRunResult OK = new RuleRunResult()
Review Comment:
## CodeQL / Constant interface anti-pattern
Type implements constant interface [RuleRunResult](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/11293)
##########
server/src/main/java/org/apache/druid/server/coordinator/rules/ShardGroupFollowup.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.server.coordinator.rules;
+
+import org.apache.druid.timeline.DataSegment;
+
+import java.util.Map;
+
+/**
+ * Signal returned by {@link Rule#run} that the entire shard group containing {@link #matchedSegment} should be
+ * inspected for uniform partial-load placement. The {@code RunRules} duty collects these and, after all rules have
+ * run, dispatches {@link PartialLoadMatcher#emptyMatch} loads to siblings that did not get a positive match from
+ * the same {@link #matcher}.
+ *
+ * <p>Used to handle asymmetric matchers (e.g. {@link ClusterGroupPartialLoadMatcher} over range-partitioned segments)
+ * where different partitions of a shard group resolve to different load specs and the broker would otherwise drop
+ * the group as incomplete via {@code PartitionHolder.isComplete()}.
+ */
+public record ShardGroupFollowup(
Review Comment:
## CodeQL / Constant interface anti-pattern
Type ShardGroupFollowup implements constant interface [RuleRunResult](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/11292)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org