curcur commented on a change in pull request #13928:
URL: https://github.com/apache/flink/pull/13928#discussion_r517785523
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/PipelinedApproximateSubpartition.java
##########
@@ -92,6 +94,11 @@ private void releaseView() {
}
}
+ @Override
+ public void finishReadRecoveredState(boolean
notifyAndBlockOnCompletion) throws IOException {
+ // The Approximate Local Recovery can not work with unaligned
checkpoint for now, so no need to recover channel state
Review comment:
Hey @rkhachatryan , thanks so much for reviewing!
Yep, what you mentioned is another place I was thinking to make the change.
But I am personally slightly preferring to override the method in
`PipelinedApproximateSubpartition.java`. The reason is that we already have a
different ResultPartitionType (Pipelined_Approximate). Put the difference in
the class implementation (`PipelinedApproximateSubpartition.java`) reminds us
this is another place where Pipelined_Approximate is different from
Pipelined(_bounded). Indeed we probably would have different implementations of
channel restore for Pipelined_Approximate (for now, it is just do nothing).
I was leaning to think of it as **a different implementation** instead of
**disabling the feature**.
There are some other places where unaligned checkpoints are not compatible
with approximate failover, not just rescaling.
The check of "approximate failover" vs "unaligned checkpoint" not be enabled
at the same time will is added in the PR Till is reviewing right now. It is
added when ResultPartitionType is decided.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
