afedulov commented on code in PR #656:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/656#discussion_r1305611024
##########
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:
I think if there is any such weird case possible, I'd prefer to uncover it.
If a user configures an interval, it is reasonable tot expect that she would
eventually check it is is firing. In that case t is arguably better not to fire
at all and potentially get a bug submitted than trigger a random thing that
matches, but is interpreted in the wrong way.
--
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]
