ARTEMIS-3763 Use margin when checking time difference between broker and database

[MM53]

I created a new PR with the changes of #4017 and added them to the current release (2.25.0) because my previous PR did not receive any updates and we need this change to use artemis in production.

If you don't like these changes please let me know then I will look for another solution.

I also noticed that you are reworking the logging of the complete project. If you think it would be better to delay this PR until the new logging is fully implemented, I will wait. Just tell me your decision.

[clebertsuconic]

As in any feature, a test would be required to guarantee sustained quality of the feature.. not just for the current but also preventing someone from ever breaking it.

[clebertsuconic]

We would need at least a Mocking test validating it. a test with a real Database (we use Derby in a lot of tests) would be my preferred option.

[MM53]

Thanks for your reply @clebertsuconic. I will add some tests in the next few days.

[clebertsuconic]

@MM53 please, add some description on the JIRA about the feature and how you're testing it?

[MM53]

@clebertsuconic I wouldn't consider this change as a new feature. It just improves the logging of the existing JDBC-persistence by making two log lines more predictable than before. The concrete issue was discussed in #4017 and also shortly described in JIRA.

I only added a new optional configuration parameter which is used to decide when a warning log should be printed out. Therefore the only effect I could verify in a test is if this warning was printed. But I couldn't find any solution to observe the logs without using some crazy reflections which will definitely break with some newer JDKs or without modifying the JdbcLeaseLock class to set a new LOGGER at runtime which doesn't seem to be a good idea either. To my mind the current tests are sufficient because they verify that artemis still starts even with the new configuration parameter. Existing logging also isn't tested at the moment.

If you have a better idea, please let me know. I am open to any suggestions.

[clebertsuconic]

@MM53 well, you're having to add configuration, it makes it a feature...

You don't know me personally but between bug and feature it's usually a relative concept.. even in real life...

Bug: Stuff is not working Feature: Stuff is not implemented.

it's just a wording difference.

Anyway, regardless, please add some detail to the JIRA on what you're changing, and why please (if you already did, that's fine.. I'm not saying you didn't).

[michaelandrepearce]

Whats difference of this and same named PR? https://github.com/apache/activemq-artemis/pull/4017?

If no difference then please check all previous points in other pr are addressed

[michaelandrepearce]

Have points raised in 4017 been addressed in this PR?

[MM53]

Yes, all suggestions of 4017 were also added to this one

[clebertsuconic]

@MM53 I'm running the entire set of tests on this branch.. if all good I will merge it...

this is just about logging a warn only if the things is outside of a configured window, right?

let me run it and I will merge it..

(I may change the .warn to use the Logging code though).

[clebertsuconic]

@MM53 instead of asking you do perform a few changes, I am doing the simple changes I was going to ask you to do, and I'm amending them as you.. you still get authorship on the git logs.. (It was just easier to do it that way).

Please take a look on the final result of the commit. look at what I did on the test where I made the class non final, and made the test to return a fake time.

I had to do this because your test was not actually failing. The assertion was passing for something else. I made usage of the AssertionLoggerHandler where we use it in a few other tests.