mxm commented on code in PR #656:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/656#discussion_r1305575829
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/SnapshotUtils.java:
##########
@@ -264,101 +264,103 @@ static boolean shouldTriggerAutomaticSnapshot(
return false;
} // automaticTriggerExpression was configured by the user
- // Try to interpret as an interval (Duration).
- boolean expressionIsAnInterval = true;
- boolean shouldTriggerInterval = false;
- try {
- shouldTriggerInterval =
- shouldTriggerIntervalBasedSnapshot(
- snapshotType, automaticTriggerExpression,
lastTrigger);
- } catch (ParseException e) {
- expressionIsAnInterval = false;
- }
-
- // Try to interpret as a cron expression.
- boolean expressionIsACron = true;
- boolean shouldTriggerCron = false;
- try {
- shouldTriggerCron =
- shouldTriggerCronBasedSnapshot(
- snapshotType, automaticTriggerExpression,
lastTrigger, Instant.now());
- } catch (ParseException e) {
- expressionIsACron = false;
- }
-
- if (!expressionIsAnInterval && !expressionIsACron) {
- LOG.warn(
- "Automatic {} triggering is configured, but the trigger
expression '{}' is neither a valid Duration, nor a cron expression.",
- snapshotType,
- automaticTriggerExpression);
- return false;
- }
+ boolean isInterval = isInterval(automaticTriggerExpression);
+ boolean isCron = isCron(automaticTriggerExpression);
// This should never happen. The string cannot be both a valid
Duration and a cron
// expression at the same time.
- if (expressionIsAnInterval && expressionIsACron) {
+ if (isInterval && isCron) {
LOG.error(
"Something went wrong with the automatic {} trigger
expression {}. This setting cannot be simultaneously a valid Duration and a
cron expression.",
snapshotType,
automaticTriggerExpression);
return false;
Review Comment:
The question is whether we want to skip triggering entirely or we choose to
go with the interval. I think semantically it is not possible for a proper cron
to be an interval, so this is is an extreme edge case. Also we fail silently
and continue operation which is also debatable.
I would just go with whatever matches first, i.e. the interval, but maybe
there is no right and wrong.
--
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]
