introduce history level auto, fixes #CAM-3444

[jangalinski]

This addresses the problem that when you configure multiple engines against one db, you have to explicitly set the right (unique) history level value, otherwise engine setup fails. WIth the new level "auto", the engine tries to guess the

• new string constant "auto"
• allow configuration with level "auto" to init() without choosing a historyLevel, this will be done during build() to avoid lifecycle mixup (db init ...)
• during construction of the ProcessEngineImpl, it checks the already configured value via the OverwriteHistoryLevelCmd and overwrites auto/null with for example
  full/HistoryLevelFull.
• afterwards, engine generation goes on as usual.
• when auto is used for the first engine that creates the schema, the default fallback is "audit"

[buildhive]

camunda BPM » camunda-bpm-platform #1545 SUCCESS
This pull request looks good
(what's this?)

[jangalinski]

After a good night sleep, I believe calling this "DetermineHistoryLevelCmd" and letting it return a history level would be more intuitive. We would still have to pass the list of available historyLevels, in case the use defined custom ones.

[buildhive]

camunda BPM » camunda-bpm-platform #1548 SUCCESS
This pull request looks good
(what's this?)

[ThorbenLindhauer]

Hi Jan,

Thanks for your pull request which looks fine from a functionality point of view.

I have two more things to ask:

1: Can you please adhere to our code style and use camel case instead of underscores in method names?
2: Could you please add a test case that asserts that when no history level is set in the db and AUDIT is chosen as default, that the engine configuration's level is updated accordingly to AUDIT?

In addition, it would be great if you could submit a pull request to the docs.camunda.org repostory adding some explanation for the AUTO level. For example, it could go here: http://docs.camunda.org/7.3/guides/user-guide/#process-engine-history-and-audit-event-log-choosing-a-history-level (source: https://github.com/camunda/docs.camunda.org/blob/master/site/src/documents/guides/user-guide/chapters/01-process-engine/06-history.html.md)

Best regards,
Thorben

[ThorbenLindhauer]

Hi Jan,

Here is one more thing: ;)

I think DetermineHistoryLevelCmd should fail when the database contains a not-null history level but the engine executing the command does not have this history level registered. With the current implementation, it would assume level AUDIT is correct (which then later fails when the history check is performed again). I think DetermineHistoryLevelCmd should already throw an exception at that point.

Also, have you considered making the AUTO logic part of the command SchemaOperationsProcessEngineBuild, in particular the method checkHistoryLevel? In case of AUTO, it could just react differently by setting the retrieved level for the current engine instead of throwing an exception. That would keep the history- and database-related logic in one place.

Cheers,
Thorben

[jangalinski]

Hi Thorben. I did some changes, please check the new commit:

• Code Style: changed (although I would recommend to consider switching to this_is_what_the_test_does(), it is much more readable than "thisIsWhatTheTestDoes".) I am only talking about tests, of course
• A test ensures that not only the DB value is overwritten but also the engine config is changed
• the logic was moved to checkHistoryLevel, good idea, now everything is in one place.
• an exception is thrown when a custom db level is not registered for the new engine

This should address your issues except documentation (will deal with that tomorrow).

[buildhive]

camunda BPM » camunda-bpm-platform #1561 SUCCESS
This pull request looks good
(what's this?)

[meyerdan]

Code Style: changed (although I would recommend to consider switching to this_is_what_the_test_does(), it is much more readable than "thisIsWhatTheTestDoes".) I am only talking about tests, of course

Hi Jan,
I agree with you. Yet currently it is not done that way and we perfer keeping things consistent as much as possible. But: if you provide a pull request changing that in all the projects, we will merge it :)
For the time being we prefer working on more features and bugfixes...

[jangalinski]

You mean, like this?

 find . -type f -name "*Test.java" -exec sed -r -i '/public void/ s/([a-z0-9])([A-Z])/\1_\L\2/g' {} \;

Should I really provide a PR? :-)

[meyerdan]

Hi Jan,

Yes that would be great and thanks for the regular expression.

There may be some more things involved, like: when using the @Deployment annotation, the name of the deployed bpmn process is automatically determined based on the method name and things like that.

And: at the moment I am only speaking for myself. I am not sure the other guys agree.
=> If you seriously want to work on this we should open a new PR / thread.

Daniel

[jangalinski]

I created https://app.camunda.com/jira/browse/CAM-4301 for further discussion. Could you perhaps fix the formatting? over and out.

[ThorbenLindhauer]

Hi Jan,

I am about to merge your contribution and make some small changes.

I think about the case, where the history is set to "auto" and the level was fetched from the database. I would prefer only overriding the "historyLevel" property, not "history" in the engine config. Then, it is possible to determine later on whether an engine has been configured with AUTO or for example AUDIT. Is that fine for you?

Cheers,
Thorben

[jangalinski]

Go ahead. I thought its best when those to attributes are in sync, but you have a good point too.

One more thing while you are on it: AUDIT is an implicit default value. Right now two or three locations use it to express default. It would be better to define a HISTORY_DEFAULT constant that references AUDIT, so we have an explicit default. Would be cleaner when one day FULL becomes the new default ....

[ThorbenLindhauer]

Hi Jan,

Thanks for your efforts in providing this feature and incorporating my feedback. I just merged it to master: d914d46

Thanks for the remark on the default history setting. I added a constant and a method that gets the default level.

Cheers,
Thorben

[jangalinski]

Nice, thanks. How do you want to continue with documentation? new PR?

[ThorbenLindhauer]

Yes, a second pull request to https://github.com/camunda/docs.camunda.org would be nice. I'll keep the JIRA issue https://app.camunda.com/jira/browse/CAM-3444 open.