pnowojski commented on a change in pull request #17331:
URL: https://github.com/apache/flink/pull/17331#discussion_r713645373
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointFailureManager.java
##########
@@ -35,6 +35,7 @@
public static final int UNLIMITED_TOLERABLE_FAILURE_NUMBER =
Integer.MAX_VALUE;
Review comment:
nit: in the first commit message I would add a bit of explanation why
you need this, for example:
```
[refactor][runtime] Supported not initialized checkpoint in
CheckpointFailureManager
This will allow us to use CheckpointFailureManager on early stages of
triggering checkpoint
when checkpoint id is not yet known.
```
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointFailureManager.java
##########
@@ -35,6 +35,7 @@
public static final int UNLIMITED_TOLERABLE_FAILURE_NUMBER =
Integer.MAX_VALUE;
public static final String EXCEEDED_CHECKPOINT_TOLERABLE_FAILURE_MESSAGE =
"Exceeded checkpoint tolerable failure threshold.";
+ private static final int NOT_INITIALIZED_CHECKPOINT = -1;
Review comment:
`NOT_INITIALIZED_CHECKPOINT` -> `NOT_INITIALIZED_CHECKPOINT_ID`?
`UNKNOWN_CHECKPOINT_ID`?
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java
##########
@@ -672,16 +672,20 @@ public void
testIOExceptionForPeriodicSchedulingWithInactiveTasks() throws Excep
}
/** Tests that do not trigger checkpoint when IOException occurred. */
- @Test
Review comment:
accidentally removed `@Test`?
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java
##########
@@ -674,7 +674,14 @@ public void
testIOExceptionForPeriodicSchedulingWithInactiveTasks() throws Excep
/** Tests that do not trigger checkpoint when IOException occurred. */
@Test
public void testTriggerCheckpointAfterIOException() throws Exception {
- testTriggerCheckpoint(new TestingIOExceptionCheckpointIDCounter(),
IO_EXCEPTION);
Review comment:
Wouldn't it be better to replace `TestingIOExceptionCheckpointIDCounter`
with `IOExceptionCheckpointStorage` everywhere?
--
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]
