"time-zone" of camunda-engine as an optional property?

[vojkog]

I had a problem when camunda-kafka-polling-client and camunda process-engine are in different time-zones.

I guess it is not an issue of practical significance, but in a testing environment (e.g. my docker setup) polling does not work correctly.

One idea (that works for me) is that after existing lines like: final SimpleDateFormat apiDateFormat = new SimpleDateFormat(API_DATE_FORMAT);

... added: apiDateFormat.setTimeZone(TimeZone.getTimeZone(<TIME-ZONE-PROP>));

I would appreaciate hearing your thoughts.

[fkoehne]

Making the source timezone configurable seems reasonable to me. What do you think, @MTwelkemeier?

Sidenote: SDFs generally make me nervous to due their lack of thread-safety. I have not yet found any threading isues here, though.

[vojkog]

First cut impl: https://github.com/moreECM/camunda-kafka-polling-client/commit/4ef0291fe58b0ce23f03397f96c5ff03da2f11bd

[vojkog]

Regarding sidenote, im my imp. SDF and API_DATE_FORMAT are now referenced from single place only : getAPIDateFormat.

[fkoehne]

We're on a conference currently - so, sorry for the delay. We will take a look at it shortly.

[MTwelkemeier]

I agree, this is a usefull enhancement. Regarding access to the configuration property I would prefer using the existing properties class de.viadee.camunda.kafka.pollingclient.config.properties.PollingProperties to not mix up property classes and direct injected configuration values.

This change focus on the REST interface. Maybe we should also think about the JDBC API to keep both in sync?

[vojkog]

Thank you for your comments.

Regarding PollingProperties class, I was trying to have a single property for runtime and repository data. Do you think that we should have something like this?

polling.repository-data.sourceTimeZone=${POLLING_SOURCE_TIME_ZONE:}  
polling.runtime-data.sourceTimeZone=${POLLING_SOURCE_TIME_ZONE:}

Yes, my focus now is on REST but certainly it would be better to keep both in sync. I will look into that.

[MTwelkemeier]

Interesting aspect. I think, since both are polled from the same source URL, we might not need different time zones.

[vojkog]

I agree that was my thinking too. Since PollingProperties are instantiated separately:

    @NestedConfigurationProperty
    private PollingProperties runtimeData = new PollingProperties();

    @NestedConfigurationProperty
    private PollingProperties repositoryData = new PollingProperties();

what do you think will be the best approach for the common property?

[vojkog]

Regarding JDBC, it seems there are no time-zone issues there. (HistoryService queries use raw Date(s) not Strings).

[MTwelkemeier]

Maybe we could place it directly within ApplicationProperties as parent structure for common settings.

[vojkog]

It seems that the better place is CamundaRestPollingProperties because JDBC is not affected and this class is already injected into CamundaRestPollingServiceImpl. Current impl. is here: https://github.com/moreECM/camunda-kafka-polling-client/commit/07e2eb304aa49b6c5eee0e2ac39afed2218357f0

Please ignore the reformated imports (IDE).

[vojkog]

Here is the cleaned-up branch (comparison with master): https://github.com/viadee/camunda-kafka-polling-client/compare/master...moreECM:sourceTimeZone2?expand=1 I would appreciate hearing your opinion on this.

[MTwelkemeier]

Great - I will have a look into it

[MTwelkemeier]

This looks great - I created a pull request and integrated the change.

[MTwelkemeier]

I also started a docker hub build to provide a new image with tag "snapshot" of current develop branch. It should be available within the next hour.

[MTwelkemeier]

Current tests are still green - maybe we should also add or enhance the rest test to also cover the new timezone feature. Do you already have some idea for junit tests?

[vojkog]

Thx, for accepting PR. I'll take a look into junit tests.

[vojkog]

While looking at JUnit tests I have found that timeZone issue is probably relevant only to Camunda versions before 7.8. https://docs.camunda.org/manual/7.7/reference/rest/overview/date-format/ :

... without milliseconds and timezone information

https://docs.camunda.org/manual/7.8/reference/rest/overview/date-format/ :

... with milliseconds and timezone information

Which versions do you plan to support, initially?

[fkoehne]

Ideally - all of them. With a focus on the recent ones, obviously.

[MTwelkemeier]

The format string used to parse the timestamps is currently hard coded. Maybe we should extract them into a configuration property with the current versions format as default?

[vojkog]

I agree unit tests have a separate constant for the same purpose, do you think tests should be refactored to use "extracted" format?

[MTwelkemeier]

If the tests are also using the configured format, we might run into issues due to embedded test engine might use a different format string.

[vojkog]

Regarding the unit tests, here is one idea: https://github.com/moreECM/camunda-kafka-polling-client/commit/6dfa861b02fb1620897dcfa515e741c2364dd9f0

Here is the ref. when date format changed to include time zone: https://github.com/camunda/camunda-bpm-platform/commit/cb86528879cceadc207c780fb2dc4a4e56437af4#diff-1dfd38231bed27089963d62e1c901a8d

[vojkog]

Here is the proposal for configurable DateFormat pattern: https://github.com/moreECM/camunda-kafka-polling-client/commit/d2fd77cf33c3110770888058be709e30122b69cc

... also, as optimization, SimpleDateFormat(ers) are now final.

[MTwelkemeier]

Regarding the unit tests, I think this could really help to provide some useful test coverage. Regarding you proposal for configurable DataFormat, I just added two small comments. But I think we can go this way.

[vojkog]

One more "idea" regarding parameters in method. getAPIDateFormat(param). We use false only once in:
this.objectMapper.setDateFormat(getAPIDateFormat(false)); All other calls are with getAPIDateFormat(true) so we do not need that parametrization if we instantiate like:
this.objectMapper.setDateFormat(new SimpleDateFormat(dateFormatPattern)); ?

[MTwelkemeier]

I think so too. This also gives the getAPIDateFormat a more clear usage context, since it is only used to create one format version.

[vojkog]

Thanks for accepting my pull request.