gyfora commented on code in PR #868:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/868#discussion_r1724486711
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -74,6 +77,11 @@ public abstract class AbstractJobReconciler<
public static final String LAST_STATE_DUMMY_SP_PATH =
"KUBERNETES_OPERATOR_LAST_STATE";
+ private final SnapshotTriggerTimestampStore checkpointTriggerTimestamps =
+ new SnapshotTriggerTimestampStore(CHECKPOINT);
+ private final SnapshotTriggerTimestampStore savepointTriggerTimestamps =
+ new SnapshotTriggerTimestampStore(SAVEPOINT);
Review Comment:
Feels like this should be a single object to wrap these two and hide the
details
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -374,13 +382,27 @@ private boolean
triggerSnapshotIfNeeded(FlinkResourceContext<CR> ctx, SnapshotTy
var resource = ctx.getResource();
var conf = ctx.getObserveConfig();
- Optional<SnapshotTriggerType> triggerOpt =
- SnapshotUtils.shouldTriggerSnapshot(resource, conf,
snapshotType);
+ var timestampStore =
+ snapshotType == SAVEPOINT
+ ? savepointTriggerTimestamps
+ : checkpointTriggerTimestamps;
+ Set<FlinkStateSnapshot> snapshots = Set.of();
+ if
(FlinkStateSnapshotUtils.isSnapshotResourceEnabled(ctx.getOperatorConfig(),
conf)) {
+ snapshots =
ctx.getJosdkContext().getSecondaryResources(FlinkStateSnapshot.class);
+ }
+
+ var lastTrigger =
timestampStore.getLastPeriodicTriggerInstant(resource, snapshots);
Review Comment:
feels like in almost all cases we don't actually need to get the secondary
resource. As long as we have the periodic timestamp already available. Can we
pass a supplier or have some other lazy logic?
--
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]
