Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove parser warning for intermediate catch timer event with a time cycle #3943

Closed
1 task done
Bragolgirith opened this issue Nov 11, 2023 · 5 comments
Closed
1 task done
Assignees
Labels
scope:core-api Changes to the core API: engine, dmn-engine, feel-engine, REST API, OpenAPI type:bug Issues that describe a user-facing bug in the project. version:7.21.0-alpha3 version:7.21.0

Comments

@Bragolgirith
Copy link
Contributor

Bragolgirith commented Nov 11, 2023

Environment (Required on creation)

Any

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket)

The BpmnParse class currently logs a warning for intermediate catch timer events with a time cycle:

ENGINE-09004 Warnings during parsing: It is not recommended to use a intermediate catch timer event with a time cycle

The warning makes sense in isolation (since a cycle is repeating, while a catch event is caught once), however there is currently at least one valid use-case for doing this, see camunda/camunda-modeler#792:

It makes sense for [cron] cycles of one :)
Because otherwise it's hard to say something like "wait for the beginning of the next day" without having to use a Java Date.

Until there's a convenient way to specify such a cron timer without a time cycle, I'd argue that the warning should be removed.

Steps to reproduce (Required on creation)

  1. Deploy a process model with intermediate catch timer event with a time cycle
  2. Observe the server log

Observed Behavior (Required on creation)

A Warning is logged:

ENGINE-09004 Warnings during parsing: It is not recommended to use a intermediate catch timer event with a time cycle

Expected behavior (Required on creation)

A Info message is logged instead.

Root Cause (Required on prioritization)

So far a Warning message is logged when an intermediate catch timer event with a time cycle is modelled. A use case exist where cron timer is better suited for a intermediate catch timer event.

Solution Ideas

  • Change the log level entry from Warning to Info message. (code)

Hints

Links

Breakdown

Pull Requests

  1. Bragolgirith

Dev2QA handover

  • Does this ticket need a QA test and the testing goals are not clear from the description? Add a Dev2QA handover comment
@Bragolgirith Bragolgirith added the type:feature Issues that add a new user feature to the project. label Nov 11, 2023
@yanavasileva yanavasileva self-assigned this Nov 13, 2023
@yanavasileva
Copy link
Member

Hi @Bragolgirith,

Thank you for reaching out to us with this.
It's a bit hard to follow the referenced modeler issue as it's quite old.
Just to confirm that I understand correctly the use case:
You would like to use a timer intermediate catch event and set the timer to run at the beginning of the next day for example?
Is it an option for you to use an expression and dynamically calculate the desired date?

Best regards,
Yana

@Bragolgirith
Copy link
Contributor Author

Bragolgirith commented Nov 16, 2023

Hi @yanavasileva ,

Just to confirm that I understand correctly the use case:
You would like to use a timer intermediate catch event and set the timer to run at the beginning of the next day for example?

Specifically, I would like to use a timer intermediate catch event and set the timer using a cron expression.

(This already works great with a time cycle, except for the warning that is being logged.)

Is it an option for you to use an expression and dynamically calculate the desired date?

Using an expression is indeed an option, but it can quickly get complicated for certain schedules, e.g. "at 10:00 o'clock but not on the weekend", which are trivial to setup with cron.

@yanavasileva
Copy link
Member

Hi @Bragolgirith,

Thank you for your patience with this and elaborating further.

We agree that a warning message can be too invasive when you have such scenario.
However, we would like to keep the message for rest of the cases. Therefore, our suggestion to to change the log level entry from Warning to Info. That way people can reduce the log level entry and not be overloaded with it in their server log file. I will adjust the ticket description and type accordingly.

What do you think? Are you willing to make a contribution on the log level change?

Best,
Yana

@yanavasileva yanavasileva added type:bug Issues that describe a user-facing bug in the project. and removed type:feature Issues that add a new user feature to the project. labels Dec 4, 2023
@Bragolgirith
Copy link
Contributor Author

Sure, I can take a stab at it after the holidays.

Bragolgirith added a commit to Bragolgirith/camunda-bpm-platform that referenced this issue Dec 26, 2023
… with a time cycle

- changes the log level entry from Warning to Info for intermediate catch timer event with a time cycle

related to camunda#3943
Bragolgirith added a commit to Bragolgirith/camunda-bpm-platform that referenced this issue Dec 26, 2023
… with a time cycle

- changes the log level entry from Warning to Info for intermediate catch timer event with a time cycle

related to camunda#3943
Bragolgirith added a commit to Bragolgirith/camunda-bpm-platform that referenced this issue Jan 27, 2024
… with a time cycle

- changes the lint warning for intermediate catch timer event with a time cycle to an INFO level log

related to camunda#3943
Bragolgirith added a commit to Bragolgirith/camunda-bpm-platform that referenced this issue Jan 27, 2024
… with a time cycle

- changes the "lint" warning for intermediate catch timer event with a time cycle to an INFO-level log

related to camunda#3943
Bragolgirith added a commit to Bragolgirith/camunda-bpm-platform that referenced this issue Jan 29, 2024
… with a time cycle

- update the logged message to contain the process definition key
- clean up the unit test

related to camunda#3943
yanavasileva pushed a commit that referenced this issue Jan 31, 2024
switch the log level entry from Warning to Info for intermediate catch timer event with a time cycle

related to #3943
@yanavasileva yanavasileva added the scope:core-api Changes to the core API: engine, dmn-engine, feel-engine, REST API, OpenAPI label Jan 31, 2024
@yanavasileva
Copy link
Member

The fix will be part of Camunda 7.21 and can be tested with the next alpha release scheduled for February.

psavidis pushed a commit that referenced this issue Mar 4, 2024
switch the log level entry from Warning to Info for intermediate catch timer event with a time cycle

related to #3943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:core-api Changes to the core API: engine, dmn-engine, feel-engine, REST API, OpenAPI type:bug Issues that describe a user-facing bug in the project. version:7.21.0-alpha3 version:7.21.0
Projects
None yet
Development

No branches or pull requests

3 participants