JBTM-3273 -> Avoid implementing InvocationHandler in jtaLogger #1590
Conversation
|
Started testing this pull request with MAIN profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=jdk8.latest,label=linux/12/ |
|
Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/12/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/12/ |
|
Started testing this pull request with PERF profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-PERF/PROFILE=PERFORMANCE,jdk=jdk8.latest,label=linux/10/ |
|
Benchmark output (please refer to the article https://developer.jboss.org/wiki/PerformanceGatesForAcceptingPerformanceFixesInNarayana for information on our testing procedures. If you just want to run a single benchmark then please refer to the README.md file in our benchmark repository at https://github.com/jbosstm/performance/tree/master/narayana JMH benchmark run (with args -t 240 -r 25 -f 2 -wi 5 -i 5) Comparison (pull request versus master) com.hp.mwtests.ts.arjuna.performance.Performance1.twoPhaseTest: 3383082.248213 vrs 3757645.428961 (0.000000%: no change) For information on the hardware config used for this PR please consult the CI job artefact hwinfo.txt or the job output If the purpose of this PR is to improve performance then there has been insufficient improvement to warrant a pass. See the previous text for the threshold (range) for passing optimization related PRs |
|
PERFORMANCE profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-PERF/PROFILE=PERFORMANCE,jdk=jdk8.latest,label=linux/10/): there were regressions in one or more of the benchmarks (see previous PR comment for details |
|
CORE profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=jdk8.latest,label=linux/12/ |
|
Since the changes are not what was expected, till then I will put this PR on Hold. |
|
Started testing this pull request with PERF profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-PERF/PROFILE=PERFORMANCE,jdk=jdk8.latest,label=linux/12/ |
|
Started testing this pull request with MAIN profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=jdk8.latest,label=linux/16/ |
|
QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/12/ |
|
Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/16/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/16/ |
|
Benchmark output (please refer to the article https://developer.jboss.org/wiki/PerformanceGatesForAcceptingPerformanceFixesInNarayana for information on our testing procedures. If you just want to run a single benchmark then please refer to the README.md file in our benchmark repository at https://github.com/jbosstm/performance/tree/master/narayana JMH benchmark run (with args -t 240 -r 25 -f 2 -wi 5 -i 5) Comparison (pull request versus master) com.hp.mwtests.ts.arjuna.performance.Performance1.twoPhaseTest: 2403901.274610 vrs 2985881.734765 (-19.491075%: regression) For information on the hardware config used for this PR please consult the CI job artefact hwinfo.txt or the job output If the purpose of this PR is to improve performance then there has been insufficient improvement to warrant a pass. See the previous text for the threshold (range) for passing optimization related PRs |
|
PERFORMANCE profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-PERF/PROFILE=PERFORMANCE,jdk=jdk8.latest,label=linux/12/): there were regressions in one or more of the benchmarks (see previous PR comment for details |
|
CORE profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=CORE,jdk=jdk8.latest,label=linux/16/ |
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/12/): AS build failed |
|
QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/16/ |
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/16/): AS build failed |
|
Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/20/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/20/ |
|
QA_JTA profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/20/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/ochaloup-test/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/1/ |
|
Started testing this pull request with QA_JTA profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/ochaloup-test/PROFILE=QA_JTA,jdk=jdk8.latest,label=linux/1/ |
|
AS_TESTS profile tests passed - Job complete http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/20/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/1/ |
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux/1/): AS build failed |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux_astests/3/ |
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux_astests/3/): AS build failed |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux_astests/5/ |
|
Started testing this pull request with AS_TESTS profile: http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk11.latest,label=linux_astests/5/ |
|
@ochaloup could you please review the code. |
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk8.latest,label=linux_astests/5/): AS build failed |
|
Thanks for the approval. I will merge it now |
|
@mmusgrov @mayankkunwar I'm sorry for not getting to this sooner. I will try better next time. |
|
Okay @ochaloup .. Thanks |
There was a problem hiding this comment.
I put down to the code few comments but as I went through this more closely, listing the changes, I can see the all reports of the recovery failure go to the class XARecoveryModule. There is no other place considered as place to report the information about recovery failure from.
So we don't need open the API out of the recovery module. This separate class RecoveryRequired (or any rather one using more fitting name) could be created as package private besides the XARecoveryModule or the flag could be placed directly to the XARecoveryModule class.
@mayankkunwar Please, refactor the code in this way or let's discuss the reasons on other decision.
| @@ -0,0 +1,14 @@ | |||
| package com.arjuna.ats.jta.logging; | |||
There was a problem hiding this comment.
I'm not sure about the placing the class under the logging package. As it informs about recovery processing it makes more sense to me to put it under recovery package. What was your reasoning about placing it here?
There was a problem hiding this comment.
Since this class is only used by jtaLogger for now, therefore I thought of placing it under com.arjuna.ats.jta.logging package would be fine for now. Maybe later if any other package requires it, then we could move it under recovery package?
There was a problem hiding this comment.
I will move it to recovery package
| @@ -0,0 +1,14 @@ | |||
| package com.arjuna.ats.jta.logging; | |||
|
|
|||
| public class RecoveryRequired { | |||
There was a problem hiding this comment.
What is your reasoning for naming the class as RecoveryRequired?
There was a problem hiding this comment.
I thought it would add meaning to the action, that user has to take after he gets the result.
|
|
||
| public class RecoveryRequired { | ||
|
|
||
| private static boolean recoveryProblems; |
There was a problem hiding this comment.
As this is a flag which is used to mark if a recovery problem occurs and this flag could be accessed from different threads and value could be set from the different threads then the code has to(!) consider that. The property should be of type atomic.
There was a problem hiding this comment.
Sorry I missed this thought. Thanks for pointing this out. I will make it thread safe.
| new jtaLogger(Logger.getMessageLogger(jtaI18NLogger.class, "com.arjuna.ats.jta"))); | ||
|
|
||
| /** | ||
| * For jtaI18NLogger, if any method, prefixed with <b>"warn_recovery"</b> is called then, |
There was a problem hiding this comment.
I think this is a good comment but I consider it should be put into the class jtaI18Logger as it's the place where new methods for logging are added (and where the methods are named). I think when it's here it's better chance to be missed.
There was a problem hiding this comment.
Sure, I will move the comments to jtaI18Logger
|
AS_TESTS profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana-AS_TESTS/PROFILE=AS_TESTS,jdk=jdk11.latest,label=linux_astests/5/): AS build failed |
https://issues.redhat.com/browse/JBTM-3273
!MAIN
!QA_JTS_JACORB
!QA_JTS_JDKORB
!QA_JTS_OPENJDKORB
AS_TESTS JDK11
!PERF
!XTS
!CORE
!BLACKTIE
!RTS
!mysql
!postgres
!db2
!oracle
!TOMCAT
!JACOCO
!LRA