pnowojski commented on a change in pull request #18845:
URL: https://github.com/apache/flink/pull/18845#discussion_r814023261
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
##########
@@ -111,6 +111,8 @@
/** The promise to fulfill once the checkpoint has been completed. */
private final CompletableFuture<CompletedCheckpoint> onCompletionPromise;
+ private final PendingCheckpointStats pendingCheckpointStats;
Review comment:
optional: I would suggest to use `Optional` instead of `@Nullable`
fields, as otherwise, just like here, you have already missed `@Nullable`
annotation. If not, please at least add the missing `@Nullable`.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java
##########
@@ -220,8 +221,13 @@ public void discardOnFailedStoring() throws Exception {
discard();
}
+ /**
+ * NOT Thread safe. This method can be called only from {@link
CheckpointCoordinator}(under the
+ * lock).
+ */
Review comment:
What's the threading model of this class at the moment? It looks to me
as all methods except of `discard()` are called from a "single thread" (single
thread = single thread or multiple threads but protected by an external lock)?
If so, maybe it makes sense to consider a construct like this:
```
@NotThreadSafe
public class CompletedCheckpoint {
public DiscardObject markAsDiscarded();
@NotThreadSafe
public static class DiscardObject {
(...);
public void discard();
}
}
```
Where both `CompletedCheckpoint` and `DiscardObject` are not thread safe on
their own, however the `DiscardObject` can be created in one thread, passed to
another, and then executed in that other thread? Kind of similar to
`BufferBuilder` and `BufferConsumer`, each not thread safe individually, but
could reside in two different threads.
--
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]
