feat(engine): support cron expression for timer cycle.

[lzgabel]

Description

As a user, I want to be able to set the cron expression for timer cycle.

Parse the given crontab expression string. The string has six single space-separated time and date fields:

   ┌───────────── second (0-59)
   │ ┌───────────── minute (0 - 59)
   │ │ ┌───────────── hour (0 - 23)
   │ │ │ ┌───────────── day of the month (1 - 31)
   │ │ │ │ ┌───────────── month (1 - 12) (or JAN-DEC)
   │ │ │ │ │ ┌───────────── day of the week (0 - 7)
   │ │ │ │ │ │          (0 or 7 is Sunday, or MON-SUN)
   │ │ │ │ │ │
   * * * * * *
   

The following rules apply:

• A field may be an asterisk (*), which always stands for "first-last". For the "day of the month" or "day of the week" fields, a question mark (?) may be used instead of an asterisk.
• Ranges of numbers are expressed by two numbers separated with a hyphen (-). The specified range is inclusive.
• Following a range (or *) with /n specifies the interval of the number's value through the range.
• English names can also be used for the "month" and "day of week" fields. Use the first three letters of the particular day or month (case does not matter).
• The "day of month" and "day of week" fields can contain a L-character, which stands for "last", and has a different meaning in each field:
  • In the "day of month" field, L stands for "the last day of the month". If followed by an negative offset (i.e. L-n), it means "nth-to-last day of the month". If followed by W (i.e. LW), it means "the last weekday of the month".
  • In the "day of week" field, L stands for "the last day of the week". If prefixed by a number or three-letter name (i.e. dL or DDDL), it means "the last day of week d (or DDD) in the month".
• The "day of month" field can be nW, which stands for "the nearest weekday to day of the month n". If n falls on * Saturday, this yields the Friday before it. If n falls on Sunday, this yields the Monday after, which also happens if n is 1 and falls on a Saturday (i.e. 1W stands for "the first weekday of the month").
• The "day of week" field can be d#n (or DDD#n), which stands for "the n-th day of week d (or DDD) in the month".

Example expressions:

• "0 0 * * * *" = the top of every hour of every day.
• "*/10 * * * * *" = every ten seconds.
• "0 0 8-10 * * *" = 8, 9 and 10 o'clock of every day.
• "0 0 6,19 * * *" = 6:00 AM and 7:00 PM every day.
• "0 0/30 8-10 * * *" = 8:00, 8:30, 9:00, 9:30, 10:00 and 10:30 every day.
• "0 0 9-17 * * MON-FRI" = on the hour nine-to-five weekdays
• "0 0 0 25 12 ?" = every Christmas Day at midnight
• "0 0 0 L * *" = last day of the month at midnight
• "0 0 0 L-3 * *" = third-to-last day of the month at midnight
• "0 0 0 1W * *" = first weekday of the month at midnight
• "0 0 0 LW * *" = last weekday of the month at midnight
• "0 0 0 * * 5L" = last Friday of the month at midnight
• "0 0 0 * * THUL" = last Thursday of the month at midnight
• "0 0 0 ? * 5#2" = the second Friday in the month at midnight
• "0 0 0 ? * MON#1" = the first Monday in the month at midnight
  of the month at midnight
• "0 0 0 * * 5L" = last Friday of the month at midnight
• "0 0 0 * * THUL" = last Thursday of the month at midnight
• "0 0 0 ? * 5#2" = the second Friday in the month at midnight
• "0 0 0 ? * MON#1" = the first Monday in the month at midnight

Related issues

closes #9673

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement
• If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

[lzgabel]

Hi @saig0. I try to fix this, please review my code and provide some comments. 🙇

[github-actions]

Unit Test Results

   785 files  +  1     785 suites  +1   1h 29m 42s ⏱️ + 2m 59s
5 496 tests  - 49  5 485 ✔️  - 53  7 💤 ±0  4 ❌ +4 
5 668 runs   - 49  5 657 ✔️  - 53  7 💤 ±0  4 ❌ +4 

For more details on these failures, see this check.

Results for commit 5d1b043. ± Comparison against base commit 159f8fe.

[saig0]

@lzgabel awesome 🎉

I'm hand it over to @korthout because I'll be on vacation for the next weeks 🌴

[korthout]

Thanks for this @lzgabel ❤️

I'd like to first find an answer to #9673 (comment) before I'll review this. Hope that's okay

[korthout]

@lzgabel As we've made a decision on the solution (i.e. we would like to continue with your original approach), I'll do my best to review this soon.

[korthout]

Thanks @lzgabel another great contribution! 🎉

❌ There are a few things to change before we can accept this 🏗️ , but I'm very excited about this PR.

🔧 To ease the review, please separate the changes over multiple commits. For example, the CRON parsing code can be in a single commit, and the code that uses the CRON parser in the engine can be another.

[lzgabel]

Hi @korthout . Sorry for the long wait. I've separated this change into multiple commits. Please check this out. 🙇

[korthout]

Hi @lzgabel I didn't find time for this today and I'm out-of-office tomorrow. I'll do my best to look at it when I'm back.

[korthout]

Hi @lzgabel Thanks for splitting up the changes into separate commits 👍

I had another look at our options, and I really prefer to use a library over copying third-party code into our repository for this.

Looking around, https://github.com/jmrozanec/cron-utils is really promising. It's trusted by several popular projects, has a small footprint, and seems actively maintained. I understood from you that there was some downside to it. As far as I can see we can make it use the SPRING CronDefinition, which would then behave the same as the copied code.

After giving it a try, I think you can easily create a simple new io.camunda.zeebe.model.bpmn.util.time.Timer implementation that implements getDueDate something like:

@Override
public long getDueDate(final long epochMillis) {
  final var cron =
      new CronParser(CronDefinitionBuilder.instanceDefinitionFor(CronType.SPRING))
          .parse("cron expression");
  return ExecutionTime.forCron(cron)
      .nextExecution(
          ZonedDateTime.ofInstant(Instant.ofEpochMilli(epochMillis), ZoneId.systemDefault()))
      .map(ZonedDateTime::toInstant)
      .map(Instant::toEpochMilli)
      .orElse(epochMillis);
}

I think this would work correctly. Please let me know what you think.

[lzgabel]

Hi @korthout . Usually this library satisfies most of our scenarios. As I mentioned before, this class library is not perfect enough and may not be able to solve some of our specific scenarios.

For example, we want to start execution at 0:00 on the third last day of each month : 0 0 L-3 * *, there is also a need for this scenario in the forum, this library does not support this scenario.

Of course, this depends on how granular the engine supports cron expression parsing, and it may require the team to participate in discussions to come to a conclusion.

[korthout]

That's strange, the README explains it does, but then it doesn't.

• Supports all cron special characters: * / , -
  • Non-standard characters L, W, LW, '?' and # are supported as well!

They are aware and plan to offer support for this next. It looks like this was introduced with Spring 5.3 1.5 years ago. We could reach out and offer our help to contribute to this as well.

What do you think?

[lzgabel]

They are aware and plan to offer support for this next. It looks like this was introduced with Spring 5.3 1.5 years ago. We could reach out and offer our help to contribute to this as well.

LGTM. 👍

We are currently working to update the codebase towards JDK 16, to ensure will be fully compatible with JDK 17 when released.

❓ Currently the class library is planning to upgrade the jdk version. So I want to ask if the jdk version (currently using jdk8) used by bpmn-model, and the indirectly dependent camunda-xml-model library, etc. also needs to be upgraded?

[korthout]

No we can't upgrade those as they are consumed by users that might depend on those Java versions. But does the crontimer really have to exist in that module? I think it's possible to introduce it to the engine module. Then the dependency exists only there.

[lzgabel]

But does the crontimer really have to exist in that module? I think it's possible to introduce it to the engine module

Hi @korthout . I really thought about it in the early days, but then I gave up because of the idea of unified module processing. 😄
So if it is introduced into the engine, we can parse it directly with the CronExpression provided by spring-context, avoiding the second introduction of the class library.

Of course a big thanks to cron-utils for such an amazing contribution. 💯

[lzgabel]

Hi @korthout . Do you have any other opinion on this? 🤔

[korthout]

Hi @lzgabel Sorry about that, I completely missed your question. 🙇

idea of unified module processing

I'm not familiar with this. Could you explain?

we can parse it directly with the CronExpression provided by spring-context, avoiding the second introduction of the class library

If you import a class from this library in the engine, you'll notice that mvn clean install -DskipTests will fail because of:

[INFO] --- maven-dependency-plugin:3.3.0:analyze-only (analyze-dependencies) @ zeebe-workflow-engine ---
[WARN] Used undeclared dependencies found:
[WARN]    org.springframework:spring-context:jar:5.3.21:compile
[INFO] Add the following to your pom to correct the missing dependencies:
[INFO]
<dependency>
  <groupId>org.springframework</groupId>
  <artifactId>spring-context</artifactId>
  <version>5.3.21</version>
</dependency>
[INFO]

We would like to keep the spring dependencies out of the engine. However, I'm fine with adding the cron-utils dependency (I'm a bit of a hypocrite I guess 😆). If we really dislike this direct dependency in the engine module, we could move the cron integration into a separate module. However, I don't see a need for this now.

[lzgabel]

I'm not familiar with this. Could you explain?

Hi @korthout . Sorry for the inconvenience caused by my unclear description. 🙇

we can parse it directly with the CronExpression provided by spring-context, avoiding the second introduction of the class library

If you import a class from this library in the engine, you'll notice that mvn clean install -DskipTests will fail because of

[INFO] --- maven-dependency-plugin:3.3.0:analyze-only (analyze-dependencies) @ zeebe-workflow-engine ---
[WARN] Used undeclared dependencies found:
[WARN]    org.springframework:spring-context:jar:5.3.21:compile
[INFO] Add the following to your pom to correct the missing dependencies:
[INFO]
<dependency>
  <groupId>org.springframework</groupId>
  <artifactId>spring-context</artifactId>
  <version>5.3.21</version>
</dependency>
[INFO]

What I want to express is that since the engine has indirectly introduced the spring-context dependency, so we don't need to introduce additional third-party libraries. So please review my last commit. 🚀

[lzgabel]

Hi @korthout . I've completed this change, please check this out. 🙇

BTW, I'll deal with code conflicts after this review. ❤️

[korthout]

This looks great! 💯 Thanks for the changes @lzgabel.

Please consider my last comment and resolve the conflict. And then we should be ready to merge. 🚀

[lzgabel]

Hi @korthout . I've resolved the conflict, please check this out. 🙇

[github-actions]

Test Results

   805 files  +    2         1 errors  804 suites  +2   1h 37m 25s ⏱️ + 6m 0s
6 321 tests +423  6 311 ✔️ +422  9 💤 ±0  1 ❌ +1 
6 507 runs  +423  6 497 ✔️ +422  9 💤 ±0  1 ❌ +1 

For more details on these parsing errors and failures, see this check.

Results for commit 04114e4. ± Comparison against base commit d672c7e.

[lzgabel]

Oops! Flaky tests encountered. 😆

[korthout]

Oops! Flaky tests encountered. 😆

Seems to be this one. Was looked at 2 days ago, but closed because unable to reproduce. Let's see if it happens more often.

• Unstable LongPollingIT.shouldActivateAndCompleteJobsInTime  #9740

[korthout]

👍 LGTM

💯 kudos for this massively important contribution! It's really great that we now support cron expressions in timer cycles ⏲️

❓ Can you please open an issue in https://github.com/camunda/camunda-platform-docs to expand the documentation on timer cycles?

❓ Can you please open an issue in https://github.com/camunda/camunda-modeler that the validation for timer cycles has changed?

• I'll add this topic to the release notes

bors merge

[zeebe-bors-camunda]

Build succeeded:

• Test summary
• Java checks
• Go linting

[lzgabel]

❓ Can you please open an issue in https://github.com/camunda/camunda-modeler that the validation for timer cycles has changed?

Hi @korthout . There are still some open issues that need our attention, such as camunda/camunda-modeler#2368, so I can't confirm the entire verification range. Could you take it over now?