[FLINK-40517][checkpointing] Aggregate cross-channel watermarks on the recovery unspilling thread for fan-in rescale - #29064
Conversation
| Watermark minWatermark = | ||
| mergeGroup.stream() | ||
| .map(VirtualChannel::getLastWatermark) | ||
| .min(Comparator.comparingLong(Watermark::getTimestamp)) |
There was a problem hiding this comment.
Since we do this on the hot path, should we avoid:
- O(N)
- stream/conversion
?
The former might be premature optimisation, but the latter I think would be an easy change
ditto: anyActive below
There was a problem hiding this comment.
Done — both switched to plain loops.
O(N)
It's the recovery unspilling path and only runs per watermark/status (not per record), so low risk. Let us kept O(N) as-is first.
| private static <T> | ||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> buildWatermarkMergeGroups( | ||
| Map<SubtaskConnectionDescriptor, VirtualChannel<T>> gateVirtualChannels, | ||
| RescaleMappings channelMapping) { | ||
| RescaleMappings oldToNewMapping = channelMapping.invert(); | ||
| Map<Integer, List<VirtualChannel<T>>> byNewChannel = new HashMap<>(); | ||
| Map<SubtaskConnectionDescriptor, Integer> keyToNewChannel = new HashMap<>(); | ||
| gateVirtualChannels.forEach( | ||
| (key, vc) -> { | ||
| int newChannelIndex = newChannelIndexOf(key, oldToNewMapping); | ||
| byNewChannel.computeIfAbsent(newChannelIndex, k -> new ArrayList<>()).add(vc); | ||
| keyToNewChannel.put(key, newChannelIndex); | ||
| }); | ||
|
|
||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> mergeGroups = new HashMap<>(); | ||
| keyToNewChannel.forEach( | ||
| (key, newChannelIndex) -> mergeGroups.put(key, byNewChannel.get(newChannelIndex))); | ||
| return mergeGroups; |
There was a problem hiding this comment.
Can't we write this transformation as
gateVirtualChannels
.entrySet()
.map(e -> newEntryWithNewKey(e)) // using newChannelIndexOf
.collect(Collectors.groupingBy(MapEntry::getKey))
?
There was a problem hiding this comment.
Applied for the grouping, thanks.
It yields Map<newChannelIndex, List<VC>>, so a second pass is still needed to re-key to old-channel key. Simplifies the front half but not the whole method.
There was a problem hiding this comment.
Thanks, NIT:
I think it can be simplified further to
RescaleMappings oldToNew = channelMapping.invert();
Map<Integer, List<VirtualChannel<T>>> byNewChannel = new HashMap<>();
Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> mergeGroups = new HashMap<>();
gateVirtualChannels.forEach(
(key, vc) -> {
List<VirtualChannel<T>> group =
byNewChannel.computeIfAbsent(
newChannelIndexOf(key, oldToNew), idx -> new ArrayList<>());
group.add(vc);
mergeGroups.put(key, group);
});
return mergeGroups;
048bc51 to
91afcb5
Compare
1996fanrui
left a comment
There was a problem hiding this comment.
thanks @rkhachatryan for the review, both are addressed.
| Watermark minWatermark = | ||
| mergeGroup.stream() | ||
| .map(VirtualChannel::getLastWatermark) | ||
| .min(Comparator.comparingLong(Watermark::getTimestamp)) |
There was a problem hiding this comment.
Done — both switched to plain loops.
O(N)
It's the recovery unspilling path and only runs per watermark/status (not per record), so low risk. Let us kept O(N) as-is first.
| private static <T> | ||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> buildWatermarkMergeGroups( | ||
| Map<SubtaskConnectionDescriptor, VirtualChannel<T>> gateVirtualChannels, | ||
| RescaleMappings channelMapping) { | ||
| RescaleMappings oldToNewMapping = channelMapping.invert(); | ||
| Map<Integer, List<VirtualChannel<T>>> byNewChannel = new HashMap<>(); | ||
| Map<SubtaskConnectionDescriptor, Integer> keyToNewChannel = new HashMap<>(); | ||
| gateVirtualChannels.forEach( | ||
| (key, vc) -> { | ||
| int newChannelIndex = newChannelIndexOf(key, oldToNewMapping); | ||
| byNewChannel.computeIfAbsent(newChannelIndex, k -> new ArrayList<>()).add(vc); | ||
| keyToNewChannel.put(key, newChannelIndex); | ||
| }); | ||
|
|
||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> mergeGroups = new HashMap<>(); | ||
| keyToNewChannel.forEach( | ||
| (key, newChannelIndex) -> mergeGroups.put(key, byNewChannel.get(newChannelIndex))); | ||
| return mergeGroups; |
There was a problem hiding this comment.
Applied for the grouping, thanks.
It yields Map<newChannelIndex, List<VC>>, so a second pass is still needed to re-key to old-channel key. Simplifies the front half but not the whole method.
| private static <T> | ||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> buildWatermarkMergeGroups( | ||
| Map<SubtaskConnectionDescriptor, VirtualChannel<T>> gateVirtualChannels, | ||
| RescaleMappings channelMapping) { | ||
| RescaleMappings oldToNewMapping = channelMapping.invert(); | ||
| Map<Integer, List<VirtualChannel<T>>> byNewChannel = new HashMap<>(); | ||
| Map<SubtaskConnectionDescriptor, Integer> keyToNewChannel = new HashMap<>(); | ||
| gateVirtualChannels.forEach( | ||
| (key, vc) -> { | ||
| int newChannelIndex = newChannelIndexOf(key, oldToNewMapping); | ||
| byNewChannel.computeIfAbsent(newChannelIndex, k -> new ArrayList<>()).add(vc); | ||
| keyToNewChannel.put(key, newChannelIndex); | ||
| }); | ||
|
|
||
| Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> mergeGroups = new HashMap<>(); | ||
| keyToNewChannel.forEach( | ||
| (key, newChannelIndex) -> mergeGroups.put(key, byNewChannel.get(newChannelIndex))); | ||
| return mergeGroups; |
There was a problem hiding this comment.
Thanks, NIT:
I think it can be simplified further to
RescaleMappings oldToNew = channelMapping.invert();
Map<Integer, List<VirtualChannel<T>>> byNewChannel = new HashMap<>();
Map<SubtaskConnectionDescriptor, List<VirtualChannel<T>>> mergeGroups = new HashMap<>();
gateVirtualChannels.forEach(
(key, vc) -> {
List<VirtualChannel<T>> group =
byNewChannel.computeIfAbsent(
newChannelIndexOf(key, oldToNew), idx -> new ArrayList<>());
group.add(vc);
mergeGroups.put(key, group);
});
return mergeGroups;
…e recovery unspilling thread for fan-in rescale
91afcb5 to
1163ef8
Compare
What is the purpose of the change
On the normal recovery path, when a fan-in rescale merges several old input channels into one new channel,
DemultiplexingRecordDeserializeraggregates the recovered non-record elements across the merged channels:it emits the min watermark (suppressed until every merged channel has one) and ACTIVE if any channel is
active. With checkpointing during recovery enabled that aggregation moves to the unspilling thread, but
ChannelStateFilteringHandlerpassed watermarks and statuses through verbatim — so a high watermark from onemerged channel advanced the operator watermark prematurely and legitimate in-flight lower-timestamp records
were dropped as late (silent data loss), and an IDLE from one merged channel wrongly idled the whole channel.
This performs the same min-watermark / any-active aggregation on the unspilling thread.
Brief change log
ChannelStateFilteringHandler;NO_RESCALEand fan-out stay verbatim.Verifying this change
This change added tests:
GateFilterHandlerTestcases for fan-in min-watermark merge (with hold-until-all) and any-active watermark-status; both fail on verbatim pass-through.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?