You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by aljoscha <gi...@git.apache.org> on 2018/03/07 11:00:46 UTC
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
GitHub user aljoscha opened a pull request:
https://github.com/apache/flink/pull/5653
[FLINK-8890] Compare checkpoints with order in CompletedCheckpoint.checkpointsMatch()
This method is used, among other things, to check if a list of restored
checkpoints is stable after several restore attempts in the ZooKeeper
checkpoint store. The order of checkpoints is somewhat important because
we want the latest checkpoint to stay the latest checkpoint.
R: @zentol
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aljoscha/flink jira-8890-compare-checkpoints-with-order
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5653.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5653
----
commit 5576c2efe776783b0c6f47ea07c8c944fc03f6b0
Author: Aljoscha Krettek <al...@...>
Date: 2018-03-07T10:58:07Z
[FLINK-8890] Compare checkpoints with order in CompletedCheckpoint.checkpointsMatch()
This method is used, among other things, to check if a list of restored
checkpoints is stable after several restore attempts in the ZooKeeper
checkpoint store. The order of checkpoints is somewhat important because
we want the latest checkpoint to stay the latest checkpoint.
----
---
[GitHub] flink issue #5653: [FLINK-8890] Compare checkpoints with order in CompletedC...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5653
Thanks @zentol and @sihuazhou! I pushed a fixup.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172838543
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
--- End diff --
It did have, but that was removed a while ago because it was not well defined, so now I have to do it like this.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172808666
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
--- End diff --
change type to List? then we could skip the copy ado the comparison directly.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172809760
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
- Set<Tuple2<Long, JobID>> firstInterestingFields =
- new HashSet<>();
+ List<Tuple2<Long, JobID>> firstInterestingFields = new ArrayList<>();
for (CompletedCheckpoint checkpoint : first) {
--- End diff --
For stability purposes we may want to check whether the input collections are actually sorted.. I know that the sole user `ZooKeeperCompletedCheckpointStore` does in fact pass sorted lists, but technically there's no guarantee at the moment.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172811578
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
--- End diff --
I'm copying because I don't know what the exact type of the input will be and also because I need to extract the set of interesting fields explicitly. WDYT?
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha closed the pull request at:
https://github.com/apache/flink/pull/5653
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172810998
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
--- End diff --
`checkpointsMatch()` can return immediately once `first.size() != second.size()`.
---
[GitHub] flink issue #5653: [FLINK-8890] Compare checkpoints with order in CompletedC...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5653
Thanks again, for the reviews!
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172807512
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
- Set<Tuple2<Long, JobID>> firstInterestingFields =
- new HashSet<>();
+ List<Tuple2<Long, JobID>> firstInterestingFields = new ArrayList<>();
for (CompletedCheckpoint checkpoint : first) {
firstInterestingFields.add(
new Tuple2<>(checkpoint.getCheckpointID(), checkpoint.getJobId()));
}
- Set<Tuple2<Long, JobID>> secondInterestingFields =
- new HashSet<>();
+ List<Tuple2<Long, JobID>> secondInterestingFields = new ArrayList<>();
--- End diff --
initialize with `second.size()`
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172811787
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
- Set<Tuple2<Long, JobID>> firstInterestingFields =
- new HashSet<>();
+ List<Tuple2<Long, JobID>> firstInterestingFields = new ArrayList<>();
for (CompletedCheckpoint checkpoint : first) {
--- End diff --
They shouldn't be sorted, they should just have the order that they have in ZooKeeper.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172812790
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
--- End diff --
Ah, I would've expected that CompletedCheckpoint has an equals method, never mind then.
---
[GitHub] flink pull request #5653: [FLINK-8890] Compare checkpoints with order in Com...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/5653#discussion_r172807476
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java ---
@@ -290,16 +289,14 @@ public static boolean checkpointsMatch(
Collection<CompletedCheckpoint> first,
Collection<CompletedCheckpoint> second) {
- Set<Tuple2<Long, JobID>> firstInterestingFields =
- new HashSet<>();
+ List<Tuple2<Long, JobID>> firstInterestingFields = new ArrayList<>();
--- End diff --
initialize with `first.size()`
---