kfaraz commented on code in PR #19843:
URL: https://github.com/apache/druid/pull/19843#discussion_r3706036318
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/PartialLoadProfile.java:
##########
@@ -93,6 +96,37 @@ public static PartialLoadProfile forLoaded(Map<String,
Object> wrappedLoadSpec,
return intern(new PartialLoadProfile(wrappedLoadSpec, fingerprint,
loadedBytes));
}
+ /**
+ * This profile in request form for {@code segment}, for reissuing to
another server the same partial load that
+ * produced it (clone catch-up, balancer move). Two things are normalized:
+ * <ul>
+ * <li>{@code loadedBytes} is dropped. A profile read back off a server
carries the footprint that server
+ * realized, which belongs to that server's announcement and not to a
request.</li>
+ * <li>The wrapper's {@link PartialLoadSpec#DELEGATE_FIELD} is replaced
with {@code segment}'s load spec. The
+ * wrapper was built when the source server was asked to load, so it
carries whatever deep-storage location
+ * the segment had then; if the payload has since been corrected or
migrated, that location may no longer
+ * exist. The scheme-specific selection and the fingerprint are what
identify the request and are preserved,
+ * so the reissued load still reconciles against the same rule.</li>
+ * </ul>
+ * {@code segment} must be the current metadata view of the segment, which
is what the coordinator's data-sources
+ * snapshot hands back. The wrapper's delegate is left alone when there is
nothing better to point it at: a segment
+ * carrying no load spec, or one whose load spec is already a partial-load
wrapper (an outbound request segment
+ * rather than the metadata view, which would otherwise nest one wrapper
inside another).
+ */
+ public PartialLoadProfile asRequestFor(DataSegment segment)
Review Comment:
Nit: rename the method to indicate that this method modifies the profile for
loading on a different server. (I feel the "clone" prefix serves the "move"
case too, since there too, we first clone the segment on target server and then
delete it from the source server)
```suggestion
public PartialLoadProfile asCloneRequestFor(DataSegment segment)
```
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/PartialLoadProfile.java:
##########
@@ -93,6 +96,37 @@ public static PartialLoadProfile forLoaded(Map<String,
Object> wrappedLoadSpec,
return intern(new PartialLoadProfile(wrappedLoadSpec, fingerprint,
loadedBytes));
}
+ /**
+ * This profile in request form for {@code segment}, for reissuing to
another server the same partial load that
+ * produced it (clone catch-up, balancer move). Two things are normalized:
+ * <ul>
+ * <li>{@code loadedBytes} is dropped. A profile read back off a server
carries the footprint that server
+ * realized, which belongs to that server's announcement and not to a
request.</li>
+ * <li>The wrapper's {@link PartialLoadSpec#DELEGATE_FIELD} is replaced
with {@code segment}'s load spec. The
+ * wrapper was built when the source server was asked to load, so it
carries whatever deep-storage location
Review Comment:
Is this needed? I think in most places, we already assume that the load spec
(and the payload in general) of a `DataSegment` is immutable.
Is that not applicable for the partial-load segments?
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CloneHistoricals.java:
##########
@@ -103,10 +112,17 @@ public DruidCoordinatorRuntimeParams
run(DruidCoordinatorRuntimeParams params)
final Set<DataSegment> sourceProjectedSegments =
sourceServer.getProjectedSegments();
final Set<DataSegment> targetProjectedSegments =
targetServer.getProjectedSegments();
- // Load any segments missing in the clone target.
+ // Load any segment that the clone target is missing, or that it holds
under a different partial-load profile
+ // than the source. Segment identity alone can't tell those apart: two
replicas of the same segment id may hold
+ // different parts of it.
for (DataSegment segment : sourceProjectedSegments) {
- if (!targetProjectedSegments.contains(segment)) {
- loadSegmentOnTargetServer(segment, targetServer, params);
+ final PartialLoadProfile sourceProfile =
sourceServer.getProjectedProfile(segment);
+ if (!targetProjectedSegments.contains(segment)
+ || !Objects.equals(
+ fingerprintOf(sourceProfile),
+ fingerprintOf(targetServer.getProjectedProfile(segment))
+ )) {
Review Comment:
Maybe put this check in a new method `shouldLoadSegmentOnTargetServer()`
##########
server/src/main/java/org/apache/druid/server/coordinator/loading/StrategicSegmentAssigner.java:
##########
@@ -184,22 +184,30 @@ public boolean moveSegment(
private boolean moveSegment(DataSegment segment, ServerHolder serverA,
ServerHolder serverB)
{
final String tier = serverA.getServer().getTier();
+
+ // A replica loaded under a partial-load rule holds only part of the
segment, so the destination has to be asked
+ // for the same parts. Read the profile up front: cancelling the load
below clears serverA's in-flight profile.
+ // `segment` is the metadata-resolved segment (see
TierSegmentBalancer.getLoadableSegment), which is what
+ // asRequestFor needs to rebase the request onto the segment's current
location.
Review Comment:
This comment should probably move into the javadoc of
`getProjectedProfile()` method.
--
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]
