[incubator-kie-issues#1612] Fix BusinessCalendar behavior with inconsistent (full/partial) set of properties on calendar.properties
#3788
Conversation
Abhitocode
changed the title
|
PR job Reproducerbuild-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u #3788 --skipParallelCheckout NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3788/1/display/redirect Test results:
Those are the test failures: PR check / Build projects / org.kie.kogito.it.JDBCOptimisticLockingIT.testAsyncWIHAssertion condition defined as a org.kie.kogito.it.PersistenceTest 1 expectation failed.Expected status code <404> but was <200>. within 10 seconds. |
| protected int getPropertyAsInt(String propertyName) { | ||
| String value = businessCalendarConfiguration.getProperty(propertyName); | ||
|
|
||
| return Integer.parseInt(value); |
There was a problem hiding this comment.
We might want to return a nicer error message if this fails:
Caused by: java.lang.NumberFormatException: For input string: ""
at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
at java.base/java.lang.Integer.parseInt(Integer.java:678)
at java.base/java.lang.Integer.parseInt(Integer.java:786)
at org.jbpm.process.core.timer.BusinessCalendarImpl.getPropertyAsInt(BusinessCalendarImpl.java:342)
| if (!errorMessage.isEmpty()) { | ||
| throw new IllegalArgumentException(errorMessage.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be good to emit a log message here that summarizes the settings, including the configured ones.
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java
Show resolved
Hide resolved
…nto incubator-kie-issues#1612 # Conflicts: # jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java # jbpm/jbpm-flow/src/test/java/org/jbpm/process/core/timer/BusinessCalendarImplTest.java
…t classes: CalendarBean is responsible of calendar.properties file. BusinessCalendarImpl is responsible of working time evaluation
There was a problem hiding this comment.
@Abhitocode hi, as I reviewed #3795 and this PR is related, I share feedback also on this PR.
01
As new potential contributor to runtimes and calendars, reading the code:
businessCal.rollCalendarAfterHolidays(calendar, false);
It is difficult to understand the false. I mean, without checking the actual method code, it is not clear what that false means. I understand that author of the code may not have this problem, but new contributors may be in similar situation as me.
What do you think about introducing two separate API methods:
businessCal.rollCalendarAfterHolidays(calendar); // businessCal.rollCalendarAfterHolidays(calendar, false);businessCal.rollCalendarAfterHolidaysAndResetTime(calendar); //businessCal.rollCalendarAfterHolidays(calendar, true);
or maybe change the boolean to some enum?
businessCal.rollCalendarAfterHolidays(calendar, RESET_TIMER.NO);
02
@Test
void rollCalendarToNextWorkingDay() {
List<Integer> workingDays = IntStream.range(Calendar.MONDAY, Calendar.SATURDAY).boxed().toList();
List<Integer> weekendDays = Arrays.asList(Calendar.SATURDAY, Calendar.SUNDAY);
Properties config = new Properties();
config.setProperty(START_HOUR, "9");
config.setProperty(END_HOUR, "17");
config.setProperty(WEEKEND_DAYS, weekendDays.stream().map(String::valueOf).collect(Collectors.joining(",")));
BusinessCalendarImpl businessCal = BusinessCalendarImpl.builder().withCalendarBean(new CalendarBean(config)).build();
workingDays.forEach(workingDay -> {
Calendar calendar = getCalendarAtExpectedWeekDay(workingDay);
businessCal.rollCalendarToNextWorkingDay(calendar, false);
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(workingDay);
});
weekendDays.forEach(weekendDay -> {
Calendar calendar = getCalendarAtExpectedWeekDay(weekendDay);
businessCal.rollCalendarToNextWorkingDay(calendar, false);
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(Calendar.MONDAY);
});
}
from reading this test ^, is rollCalendarToNextWorkingDay actually rolling? It seems we roll workingDay and after rolling we compare for equality with the original value. I would expect if we roll, that new value is +1. I mean I would expect sth like:
workingDays.forEach(workingDay -> {
Calendar calendar = getCalendarAtExpectedWeekDay(workingDay);
businessCal.rollCalendarToNextWorkingDay(calendar, false);
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(workingDay + 1);
});
Sorry if my feedback is not useful, or it proves my lack of knowledge about the domain. Thank you for your work.
|
@gitgabrio, Apologies for my incapability and mistake, I think I have a mistaken it with other test which was resolved. I checked the test again at multiple time periods, It did not fail. Hence I re-enabled it. Failures
Here timer is actually triggered after 1 sec but skips the job saying no process for pid, couldn't find the exact reason. |
…1612 # Conflicts: # jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java # jbpm/jbpm-flow/src/test/java/org/jbpm/process/core/timer/BusinessCalendarImplTest.java
|
@Abhitocode About the failing test (disabled/renabled), please double-double check it - because it can have a fuzzy behavior depending on time of execution. |
| if (startHour < endHour) { | ||
| rollCalendarToDailyWorkingHour(toRoll, startHour, endHour); | ||
| } else { | ||
| rollCalendarToNightlyWorkingHour(toRoll, startHour, endHour); |
There was a problem hiding this comment.
@Abhitocode
Until apache/incubator-kie-issues#1651 is not done, it is needed to throw an exception here, or anyway to manage it, but that branch should not be executed
There was a problem hiding this comment.
@Abhitocode - you should not remove it - you have to manage it
As the code currently is, it is possible to get here with startHour > endHour, and so this use case has to properly be dealt with - and the way it is to throw and log exception.
|
PR job Reproducerbuild-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u #3788 --skipParallelCheckout NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3788/19/display/redirect Test results:
Those are the test failures: org.jbpm.process.core.timer.BusinessCalendarImplTest.rollCalendarAfterHolidaysexpected: 343 but was: 342 |
|
@gitgabrio, After debugging the issue with the failing Business Calendar example test Logs for old business calendar implementation Logs for new Business calendar implementation
Comparing the logs from the old and new Business Calendar implementation there is difference in thread execution:
The SignalProcessInstanceOnExpiredTimer is executed on
The SignalProcessInstanceOnExpiredTimer is executed on This change in thread execution order may be causing the process to miss key events. However, I'm not sure why this change in thread behavior is occurring.
These could be the reasons for test failure in the ci and if its a blocker for merge, I'm not exactly sure and couldn't find exact solution. I would need help solving this. @martinweiler @pefernan, any suggestions/solutions would be helpful. |
|
@Abhitocode |
…nto incubator-kie-issues#1612
…, since the minute/second management is based on "add" operation
|
PR job Reproducerbuild-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u #3788 --skipParallelCheckout NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3788/21/display/redirect Test results:
Those are the test failures: org.kie.kogito.codegen.tests.PublishEventIT.testServiceTaskProcessWithErrorExpected size: 1 but was: 2 in: [ProcessInstanceStateDataEvent {specVersion=1.0, id='528dbbd9-0cbe-4d19-bfa5-0643e1315c94', source=http://myhost/TimerProcess, type='ProcessInstanceStateDataEvent', time=2024-12-05T09:35:30.403030430-05:00, subject='null', dataContentType='application/json', dataSchema=null, data=ProcessInstanceStateEventBody [eventDate=Thu Dec 05 09:35:30 EST 2024, eventUser=null, eventType=1, processId=defaultPackage.TimerProcess, processVersion=1.0, processType=BPMN, processInstanceId=47b98999-f3c7-4f8d-b5e9-224df514df8d, businessKey=null, processName=TimerProcess, parentInstanceId=null, rootProcessId=null, rootProcessInstanceId=null, state=1, roles=null], kogitoProcessInstanceId='47b98999-f3c7-4f8d-b5e9-224df514df8d', kogitoRootProcessInstanceId='null', kogitoProcessId='defaultPackage.TimerProcess', kogitoRootProcessId='null', kogitoAddons='test', kogitoIdentity='null', extensionAttributes={kogitoproctype=BPMN, kogitoprocinstanceid=47b98999-f3c7-4f8d-b5e9-224df514df8d, kogitoprocist=1, kogitoprocversion=1.0, kogitoprocid=defaultPackage.TimerProcess, kogitoaddons=test}}, ProcessInstanceStateDataEvent {specVersion=1.0, id='c5b5c68f-9af4-4ad6-b964-0a16c35bb199', source=http://myhost/ServiceProcessDifferentOperations, type='ProcessInstanceStateDataEvent', time=2024-12-05T09:35:30.406547943-05:00, subject='null', dataContentType='application/json', dataSchema=null, data=ProcessInstanceStateEventBody [eventDate=Thu Dec 05 09:35:30 EST 2024, eventUser=null, eventType=2, processId=ServiceProcessDifferentOperations, processVersion=1.0, processType=BPMN, processInstanceId=45f00adb-a2ee-4e7f-ba32-b2680fc9835b, businessKey=null, processName=Service Process, parentInstanceId=null, rootProcessId=null, rootProcessInstanceId=null, state=2, roles=null], kogitoProcessInstanceId='45f00adb-a2ee-4e7f-ba32-b2680fc9835b', kogitoRootProcessInstanceId='null', kogitoProcessId='ServiceProcessDifferentOperations', kogitoRootProcessId='null', kogitoAddons='test', kogitoIdentity='null', extensionAttributes={kogitoproctype=BPMN, kogitoprocinstanceid=45f00adb-a2ee-4e7f-ba32-b2680fc9835b, kogitoprocist=2, kogitoprocversion=1.0, kogitoprocid=ServiceProcessDifferentOperations, kogitoaddons=test}}] |
|
@yesamer @pefernan @pibizza @martinweiler @jomarko |
|
sure, will do re-review soon |
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java
Outdated
Show resolved
Hide resolved
jbpm/jbpm-flow/src/main/java/org/jbpm/process/core/timer/BusinessCalendarImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@Abhitocode this is generating too much logging output at DEBUG level. While I understand that some of the logging could be helpful to troubleshoot any miscalculations, I think we should move these to TRACE. DEBUG level logging is widely used to troubleshoot any workflow execution related issue, and I don't think we need this level of detail for all these cases.
Please check out https://gist.github.com/martinweiler/ae6a24eef68968794de86d8bc77d0b67 for a suggestion on how to change the logging. Thanks!
There was a problem hiding this comment.
LGTM - thank you @Abhitocode and @gitgabrio and everyone involved
| } | ||
|
|
||
| @Test | ||
| public void testCalculateTimeDaysHoursMinutesHolidays() { | ||
| void calculateBusinessTimeAsDateBeforeWorkingHourWithDelayFineGrained() { |
There was a problem hiding this comment.
I like the structure of this test calculateBusinessTimeAsDateBeforeWorkingHourWithDelayFineGrained. I can understand its goal even without deep knowwledge of BusinessCalendarish code.
Tests, that reduce the code repetition using commonCalculateBusinessTimeAsDateAssertBetweenHours are not easy to read and understand for me. My comment is, that more natural language === more smaller methods would help to understand the tests. However I am not going to block the PR as I am not sure if we have resources to address my point.
There was a problem hiding this comment.
jbpm/jbpm-flow/src/test/java/org/jbpm/process/core/timer/CalendarBeanFactoryTest.java
Show resolved
Hide resolved
…istent (full/partial) set of properties on `calendar.properties` (apache#3788) * incubator-kie-issues-1612 * incubator-kie-issues-1612 * incubator-kie-issues-1612 * [incubator-kie-issues#1612] TODO setup * [incubator-kie-issues#1612] Separating different concerns in different classes: CalendarBean is responsible of calendar.properties file. BusinessCalendarImpl is responsible of working time evaluation * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Example test * incubator-kie-issues-1612 * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Extend test coverage. Minor refactoring related to it. * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Comment on tests * [incubator-kie-issues#1612] Minor fixes * [incubator-kie-issues#1612] Extend test coverage. Minor refactoring related to it. * [incubator-kie-issues#1612] Minor refactoring * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Fixing tests. Including incubator-kie-issues#1648 fix * [incubator-kie-issues#1612] WIP - implementing nightly hour * [incubator-kie-issues#1612] WIP - Simplify test - moving tested methods to static * [incubator-kie-issues#1612] WIP - Cleanup * [incubator-kie-issues#1612] Working tests. * incubator-kie-issues-1612 * incubator-kie-issues-1612 * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Fixed logging * [incubator-kie-issues#1612] Fixed minute/second reset on calendarRolling * [incubator-kie-issues#1612] Fixed assertiont JUnit5 -> assertj * [incubator-kie-issues#1612] Fixed test * incubator-kie-issues-1612 * incubator-kie-issues-1612 * [incubator-kie-issues#1612] Avoid minute/second reset on rolling hour, since the minute/second management is based on "add" operation * [incubator-kie-issues#1612] Fix naming * [incubator-kie-issues#1612] Add minute / second test/fix * [incubator-kie-issues#1612] Extend test coverage * updated logging * logger update * logger update --------- Co-authored-by: Gabriele-Cardosi <gabriele.cardosi@ibm.com>
|
@gitgabrio @Abhitocode I checked the PR (sorry for the delay) and it looks good to me, thank you for your effort. |
|
@yesamer , right, example should be updated. Will update it once done with enablement. |
Modified BusinessCalendarImpl class to include validation for calendar properties preventing misconfigurations.
Made business.hours.per.day calculated value based on start hour and end hour.
Added validations for properties to throw exceptions when calendar.properties file is misconfigured.
Added tests in BusinessCalendarImplTest to verify the modified intialization logic.
Closes: apache/incubator-kie-issues#1612
NOTE: Work in progress, modifying tests and adding new test in BusinessCalendarImplTest aligning the new changes w.r.t CaleandarBean.