Fix Unintuitive Behavior #573
Conversation
| @@ -15,7 +15,7 @@ export enum SchedulerMode { | |||
|
|
|||
| export class SchedulerConfig { | |||
| crontab: string = '* * * * *'; // Every minute | |||
| mode ?: SchedulerMode = SchedulerMode.ExactlyOncePerInterval; | |||
| mode ?: SchedulerMode = SchedulerMode.ExactlyOncePerIntervalWhenActive; | |||
There was a problem hiding this comment.
I wonder if we should just get rid of the default, make people think which one they want. It's a pretty significant difference.
I wonder about communicator retries also... it's pretty fundamental.
There was a problem hiding this comment.
To eliminate the breaking change, I'm going to revert the communicator retries change and instead log exceptions retried by communicators so they aren't swallowed.
There was a problem hiding this comment.
For the scheduled workflows, it never seems to have worked right anyways, so I'm fine changing the default and making sure it works as intended.
There was a problem hiding this comment.
I don't want to have no default because that's an even bigger breaking change, and now this PR has no breaking changes at all (except to what's already broken).
This fixes unintuitive behaviors that sparked user complaints:
Scheduled workflows now default to
ExactlyOncePerIntervalWhenActive. This means that if an executor is started after being deactivated for a long time, it does not try to "catch up" on past scheduled workflows. This is paired with a related bugfix (Test the workflow makeup modes #577) so this behavior and flag now work as intended.Communicator retries now clearly emit error messages that explain what's going on, so they're no longer swallowed.
Fixed an issue where workflow errors were logged twice: once by the workflow and once by the handler.