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` because 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 (not just rescaling) where unaligned checkpoints
are not compatible with approximate failover.
The check of "approximate failover" vs "unaligned checkpoint" can 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]
