JBTM-3273 -> Implemented code review comments #1600

mayankkunwar · 2020-04-16T11:08:18Z

This PR is to implement missed code review comments on another PR.

MAIN QA_JTA AS_TESTS

!TOMCAT !RTS !JACOCO !XTS !QA_JTS_JACORB !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !BLACKTIE !PERF !LRA !NO_WIN !DB_TESTS !mysql !db2 !postgres !oracle

jbosstm-bot · 2020-04-16T11:44:47Z

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/49/

jbosstm-bot · 2020-04-16T13:27:11Z

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/49/

ArjunaJTA/jta/classes/com/arjuna/ats/jta/recovery/RecoveryRequired.java

mmusgrov · 2020-04-16T13:58:35Z

ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/recovery/arjunacore/XARecoveryModule.java

-import com.arjuna.ats.jta.recovery.XAResourceOrphanFilter;
-import com.arjuna.ats.jta.recovery.XAResourceRecovery;
-import com.arjuna.ats.jta.recovery.XAResourceRecoveryHelper;
+import com.arjuna.ats.jta.recovery.*;


We can avoid the new RecoveryRequired class and just add an AtomicBoolean to this class instead.

We have a checkstyle plugin which is skipped by default because we have a lot of old files, that we don't want to change, which would fail the compilation. But on new code we do encourage/advise that it should follow our checkstyle rules.

To see if your code follows our style guide you have two options:

Enable the checkstyle plugin via the pom file, or

Reference the rule file from your IDE settings (for Intelij that would be Settings -> Other Settings -> Checkstyle) and we use the wildfly rule set.

We can avoid the new RecoveryRequired class and just add an AtomicBoolean to this class instead.

Okay, I will remove the new RecoveryRequired class and add AtomicBoolean

We have a checkstyle plugin which is skipped by default because we have a lot of old files, that we don't want to change, which would fail the compilation. But on new code we do encourage/advise that it should follow our checkstyle rules.

To see if your code follows our style guide you have two options:

Enable the checkstyle plugin via the pom file, or

Reference the rule file from your IDE settings (for Intelij that would be Settings -> Other Settings -> Checkstyle) and we use the wildfly rule set.

Maybe checkstyle changes can be done on separate PR?

The checkstyle issue I commented on was introduced as part of this PR so a separate PR would not make sense.

I admit that having to manually run the checkstyle plugin is not optimal. But I do not see an alternative to the reason I gave:

We have a checkstyle plugin which is skipped by default because we have a lot of old files, that we don't want to change, which would fail the compilation. But on new code we do encourage/advise that it should follow our checkstyle rules.

@ochaloup We have had this problem of skipping the checkstyle plugin on two PRs this week, it mainly affects new/occasional contributors. Do you have any thoughts on how we could improve it (other than fixing all of the historical code).

I see that your latest push has fixed the checkstyle problem I reported.
But I will not mark the conversation as resolved yet since I have an outstanding question for Ondra.

Sorry for not reacting this sooner I wanted but I haven't got to this. I can see the discussion was moved to chat. I replied there.

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest2.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryScanTest.java

jbosstm-bot · 2020-04-16T15:20:01Z

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/50/

jbosstm-bot · 2020-04-16T15:37:17Z

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/51/

ochaloup

I have few points about exposing the functionality (aka. opening the API) to the outer world. I'm not sure if it's really needed. Let me know your thoughts.

ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/recovery/arjunacore/XARecoveryModule.java

ArjunaJTA/jta/classes/com/arjuna/ats/jta/logging/jtaI18NLogger.java

jbosstm-bot · 2020-04-16T17:07:08Z

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/50/

jbosstm-bot · 2020-04-16T17:19:05Z

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/51/

jbosstm-bot · 2020-04-17T07:12:35Z

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/54/

jbosstm-bot · 2020-04-17T07:19:27Z

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/18/

jbosstm-bot · 2020-04-17T07:26:00Z

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/54/

jbosstm-bot · 2020-04-17T08:50:44Z

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

jbosstm-bot · 2020-04-17T09:07:18Z

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/54/

jbosstm-bot · 2020-04-17T10:36:32Z

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

ochaloup

Thanks @mayankkunwar! This seems good to me.
@mmusgrov would you have some more points on this or it could be considered as ready for merge?

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest2.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryScanTest.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest2.java

ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryScanTest.java

mmusgrov · 2020-04-17T14:10:14Z

Thanks @mayankkunwar! This seems good to me.
@mmusgrov would you have some more points on this or it could be considered as ready for merge?

I had a couple comments for Mayank. And there was one for you about our checkstyle plugin.

jbosstm-bot · 2020-04-17T14:37:30Z

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/21/

jbosstm-bot · 2020-04-17T14:45:08Z

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/57/

jbosstm-bot · 2020-04-17T14:53:28Z

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/57/

mmusgrov · 2020-04-17T14:57:35Z

Looks good. @ochaloup I will merge this one.

jbosstm-bot · 2020-04-17T16:14:56Z

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

jbosstm-bot · 2020-04-17T16:39:01Z

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/57/

jbosstm-bot · 2020-04-17T17:36:01Z

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/21/): AS build failed

mayankkunwar requested review from mmusgrov and ochaloup April 16, 2020 11:08

mmusgrov requested changes Apr 16, 2020

View reviewed changes

mayankkunwar force-pushed the JBTM-3273 branch from 8b4a447 to 8cd324b Compare April 16, 2020 15:12

mayankkunwar force-pushed the JBTM-3273 branch from 6985930 to 824af49 Compare April 16, 2020 15:23

ochaloup suggested changes Apr 16, 2020

View reviewed changes

mayankkunwar force-pushed the JBTM-3273 branch from f127c53 to bf17d29 Compare April 17, 2020 05:05

ochaloup approved these changes Apr 17, 2020

View reviewed changes

mmusgrov requested changes Apr 17, 2020

View reviewed changes

JBTM-3273 -> Implemented code review comments

e9d7c53

mayankkunwar force-pushed the JBTM-3273 branch from 8d545e5 to e9d7c53 Compare April 17, 2020 14:30

mmusgrov approved these changes Apr 17, 2020

View reviewed changes

mmusgrov merged commit cf8e1ab into jbosstm:master Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JBTM-3273 -> Implemented code review comments #1600

JBTM-3273 -> Implemented code review comments #1600

mayankkunwar commented Apr 16, 2020 •

edited by ochaloup

Loading

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

mmusgrov Apr 16, 2020

mmusgrov Apr 16, 2020

mayankkunwar Apr 16, 2020

mayankkunwar Apr 16, 2020

mmusgrov Apr 17, 2020 •

edited

Loading

mmusgrov Apr 17, 2020

ochaloup Apr 21, 2020

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

ochaloup left a comment

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

ochaloup left a comment •

edited

Loading

mmusgrov commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

mmusgrov commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

JBTM-3273 -> Implemented code review comments #1600

JBTM-3273 -> Implemented code review comments #1600

Conversation

mayankkunwar commented Apr 16, 2020 • edited by ochaloup Loading

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

mmusgrov Apr 16, 2020

Choose a reason for hiding this comment

mmusgrov Apr 16, 2020

Choose a reason for hiding this comment

mayankkunwar Apr 16, 2020

Choose a reason for hiding this comment

mayankkunwar Apr 16, 2020

Choose a reason for hiding this comment

mmusgrov Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

mmusgrov Apr 17, 2020

Choose a reason for hiding this comment

ochaloup Apr 21, 2020

Choose a reason for hiding this comment

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

ochaloup left a comment

Choose a reason for hiding this comment

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 16, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

ochaloup left a comment • edited Loading

Choose a reason for hiding this comment

mmusgrov commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

mmusgrov commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

jbosstm-bot commented Apr 17, 2020

mayankkunwar commented Apr 16, 2020 •

edited by ochaloup

Loading

mmusgrov Apr 17, 2020 •

edited

Loading

ochaloup left a comment •

edited

Loading