[JBTM-3541] adjustments on LRA log messages

[ochaloup]

https://issues.redhat.com/browse/JBTM-3541

LRA
!MAIN !CORE !QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS !PERF NO_WIN !RTS !AS_TESTS !TOMCAT !JACOCO

[jbosstm-bot]

Started testing this pull request with LRA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/232/

[mmusgrov]

How are you testing that the debug output is produced, don't we need a log4j or some other dependency such as:

    <dependency>
      <groupId>log4j</groupId>
      <artifactId>log4j</artifactId>
      <version>${version.log4j}</version>
      <scope>provided</scope>
    </dependency>

Also there is at least one other example using LRARecord so we should either stay with LRARecord or we should change them all.

And the test for debug on the stacktrace should be inside the test for info on the previous line.

And while you are looking at the logging, can you also change the following:

-            LRALogger.logger.tracef("%s: Participant id: %s, LRA id: %s, reason: %s, state: %s, accepted: %b",
+            LRALogger.logger.tracef("%s: LRA id: %s, Participant id: %s, reason: %s, state: %s, accepted: %b",

[jbosstm-bot]

LRA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/232/

[mmusgrov]

There are also a number of references to LRARecord in the LraI18nLogger file so, again, either change them all or none of them but since they are just log messages I don't have an opinion either way as long as we are consistent.

[ochaloup]

@mmusgrov

How are you testing that the debug output is produced, don't we need a log4j or some other dependency such) I don't test the output and we don't need the log4j as we works with org.jboss.logging:jboss-logging and it wraps the log implementation provided by runtime. Though the runtime has to have access to jboss-logging library otherwise the code fails with CNFE.

Also there is at least one other example using LRARecord ) you're right, there is one. Thank you. I fixed that.

And the test for debug on the stacktrace should be inside the test for info on the previous line) Do you mean to add the test for INFO level, like: if (LRALogger.logger.isInfoEnabled()) ...? The code usually does not use the check for info as info is quite normal level to be logged and that check was not part of the original code. But I can add it. Would you think so?/

can you also change the following) sure

number of references to LRARecord in the LraI18nLogger) I found the names of the logger methods but not the messages that are printed. I changed those two to lra participant record.

either change them all or none of them) I changed them all.

Thank you for review. I tried to address your notes (comments above). Please, take another look.
If you consider this PR as a problematic change I may withdraw it.

[jbosstm-bot]

Started testing this pull request with LRA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/233/

[jbosstm-bot]

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/233/): LRA Test failed with failures in arq profile

[ochaloup]

I'm going to rerun the CI. The failures is FailedLRAIT.testCompleteFailed:150->validateStateAndRemove:319 Could not remove log expected:<204> but was:<500> which intermittent to CI on removing the log from store.

[jbosstm-bot]

Started testing this pull request with LRA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/234/

[jbosstm-bot]

LRA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/234/

[mmusgrov]

How are you testing that the debug output is produced, don't we need a log4j or some other dependency such) I don't test the output and we don't need the log4j as we works with org.jboss.logging:jboss-logging and it wraps the log implementation provided by runtime. Though the runtime has to have access to jboss-logging library otherwise the code fails with CNFE.

how are you testing that the debug output is produced?

And the test for debug on the stacktrace should be inside the test for info on the previous line) Do you mean to add the test for INFO level, like: if (LRALogger.logger.isInfoEnabled()) ...? The code usually does not use the check for info as info is quite normal level to be logged and that check was not part of the original code. But I can add it. Would you think so?/

The code logs an info message and then immediately on the next line it checks for debug log level so it makes more sense, in this instance of the logging, to surround the info log message with a level check; if the log level is above info and you check for info level then your the test for debug enabled will be skipped if the info message is inside an if block, ie the code will avoid needing to execute a conditional test.

number of references to LRARecord in the LraI18nLogger) I found the names of the logger methods but not the messages that are printed. I changed those two to lra participant record.

Not sure what you are saying here but my point was that the method names refer to LRARecord and this PR is all about cleaning/expunging references to LRARecord.

[ochaloup]

how are you testing that the debug output is produced?

We do not test if the debug output is produced. It's not done in any part of the Narayana code or WildFly. I don't think it's necessary. Do you consider it should be tested here?

The code logs an info message and then immediately on the next line it checks for debug log level so it makes more sense, in this instance of the logging, to surround the info log message with a level check; if the log level is above info and you check for info level then your the test for debug enabled will be skipped if the info message is inside an if block, ie the code will avoid needing to execute a conditional test.

I changed the code to have the check for info level which wraps the check for debug level.

Not sure what you are saying here but my point was that the method names refer to LRARecord and this PR is all about cleaning/expunging references to LRARecord.

I understand. So I think the code could be adjusted in this way now.

[jbosstm-bot]

Started testing this pull request with LRA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/236/

[jbosstm-bot]

LRA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/236/

[mmusgrov]

how are you testing that the debug output is produced?

We do not test if the debug output is produced. It's not done in any part of the Narayana code or WildFly. I don't think it's necessary. Do you consider it should be tested here?

No but I wanted to know how you are verifying the log output (how do you enable, for example, trace level logging)?

[jbosstm-bot]

QA_JTS_OPENJDKORB profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTS_OPENJDKORB,jdk=jdk8.latest,label=linux/238/): Narayana rebase on master failed. Please rebase it manually

[ochaloup]

@mmusgrov oh, I see, sorry I didn't understand.
The debug/trace can be switched on by standard manner in the application. When coordinator is run within the Quarkus then it's https://quarkus.io/guides/logging and or for WildFly it's https://docs.wildfly.org/24/Admin_Guide.html#Logging