JBTM-3911 Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method #2293

marcosgopen · 2024-08-26T15:11:13Z

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

previous title was 'Remove redundant lock monitor from LRAService'

!CORE !AS_TESTS !RTS !JACOCO !XTS !QA_JTA !QA_JTS_OPENJDKORB !PERFORMANCE LRA !DB_TESTS

jbosstm-bot · 2024-08-26T15:13:52Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/481/

jbosstm-bot · 2024-08-26T15:14:15Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/481/

jbosstm-bot · 2024-08-26T15:17:18Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/481/

jbosstm-bot · 2024-08-26T15:39:16Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/481/

xstefank · 2024-08-27T05:21:00Z

...rdinator/src/test/java/io/narayana/lra/coordinator/domain/model/LRAWithParticipantsTest.java

+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Response;
+
+public class LRAWithParticipantsTest {


@marcosgopen this is very much like copy-paste of LRATest. Refactor it so we don't need to always change the same thing in two different classes please.

Thanks Martin, I cleaned and refactored the test.

xstefank · 2024-08-27T05:23:56Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java

@@ -328,7 +328,7 @@ public int leave(URI lraId, String compensatorUrl) {
        }
    }

-    public synchronized int joinLRA(StringBuilder recoveryUrl, URI lra, long timeLimit,
+    public int joinLRA(StringBuilder recoveryUrl, URI lra, long timeLimit,


As we discussed offline, I'm not sure what all the possible implications of removing synchronized from this method could be. I would like to hear from @mmusgrov why it was synchronized in the first place.

Good idea, I put the on hold label.

@marcosgopen I added a comment to the JBTM indicating that the tests are good but I'm not convinced that removing the monitor is the correct approach. Personally I would keep the tests but withdraw the fix and work on a different fix for the lock contention, do you mind if I look into the alternate approach?

I updated the PR, I am still removing the synchronized but adding the ReentrantLock during the enlistment. Could you have a look at this solution @mmusgrov ?

The general technique used in the PR is a good one to break the lock contention. I did have some comments about other parts of the PR though so I have asked for changes.

jbosstm-bot · 2024-08-27T08:58:34Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/482/

jbosstm-bot · 2024-08-27T08:58:54Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/482/

jbosstm-bot · 2024-08-27T09:02:24Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/482/

jbosstm-bot · 2024-08-27T09:25:01Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/482/

mmusgrov · 2024-09-04T09:32:12Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java

@@ -328,7 +328,7 @@ public int leave(URI lraId, String compensatorUrl) {
        }
    }

-    public synchronized int joinLRA(StringBuilder recoveryUrl, URI lra, long timeLimit,
+    public int joinLRA(StringBuilder recoveryUrl, URI lra, long timeLimit,


@marcosgopen I added a comment to the JBTM indicating that the tests are good but I'm not convinced that removing the monitor is the correct approach. Personally I would keep the tests but withdraw the fix and work on a different fix for the lock contention, do you mind if I look into the alternate approach?

marcosgopen · 2024-09-04T10:06:48Z

That works for me @mmusgrov , I put the on hold label and I will re-adapt the PR so to keep only the test.

jbosstm-bot · 2024-09-04T15:58:56Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/483/

jbosstm-bot · 2024-09-04T15:59:07Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/483/

jbosstm-bot · 2024-09-04T16:02:55Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/483/

jbosstm-bot · 2024-09-04T16:25:16Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/483/

jbosstm-bot · 2024-09-05T11:33:43Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/484/

jbosstm-bot · 2024-09-05T11:33:46Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/484/

jbosstm-bot · 2024-09-05T11:37:31Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/484/

marcosgopen · 2024-09-05T11:40:34Z

...inator/src/test/java/io/narayana/lra/coordinator/tools/osb/mbean/ObjStoreBrowserLRATest.java

@@ -90,7 +90,8 @@ public void lraMBean() throws Exception {

    @Test
    public void lraMBeanRemoval() throws Exception {
-        LongRunningAction lra = new LongRunningAction(new Uid());
+        String lraUrl = "http://localhost:8080/lra";


Previous way of creating the LRA was not working anymore, lraID was not initialized and therefore was causing a NullPointerException at https://github.com/jbosstm/narayana/blob/main/rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java#L107. So changing how it is initialized.

The NullPointerException path from tryLockTransaction(URI) that I observed was during the addition of tryLockTransaction needed for enlistParticipant so I expect changing this test is OK. Although the LongRunningAction(Uid) constructor is added for MBean listing, it is used here to initialize a useable LongRunningAction and so it is reasonable to change it to use one of the constructors that appear expected for that purpose.

I will take a look next week (I'm on PTO today but wanted to get this PR reviewed asap).

But don't let this comment block the PR.

The constructor should only be used when loading an LRA from the ObjectStore. So after constructing the LRA the expectation is that restore_state will be called so that the URI for the LRA gets initialised. Clearly the code is brittle so it would be nice to firm it up in this area - I have raised JBTM-3919 so that we don't loose site of it.

Thanks Mike!

jbosstm-bot · 2024-09-05T11:59:12Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/484/

jbosstm-bot · 2024-09-10T10:00:41Z

LRA profile tests failed (https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/486/): narayana build failed

jbosstm-bot · 2024-09-10T10:00:41Z

LRA profile tests failed (https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/486/): narayana build failed

jbosstm-bot · 2024-09-10T10:13:44Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/487/

jbosstm-bot · 2024-09-10T10:14:00Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/487/

jbosstm-bot · 2024-09-10T10:17:14Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/487/

jbosstm-bot · 2024-09-10T10:39:58Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/487/

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LongRunningAction.java

mmusgrov · 2024-09-10T16:25:07Z

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LongRunningAction.java

-        if (participant != null) {
-            participant.setCompensatorData(compensatorData);
-            return participant; // must have already been enlisted
+            long timeLimit, String compensatorData) throws UnsupportedEncodingException {


Superfluous reformatting.

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LongRunningAction.java

rts/lra/service-base/src/main/java/io/narayana/lra/logging/LraI18nLogger.java

mmusgrov · 2024-09-11T08:15:47Z

@marcosgopen Will you also squash the commits prior to merging please.

marcosgopen · 2024-09-11T09:51:01Z

@mmusgrov PR updated and commits squashed. Thanks for your review.
WDYT if we change the JBTM title with a more consistent one? e.g. "Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method'

mmusgrov

PR looks good, sorry for not explaining ERROR vs WARN logging levels more clearly.

mmusgrov · 2024-09-11T10:50:35Z

@mmusgrov PR updated and commits squashed. Thanks for your review. WDYT if we change the JBTM title with a more consistent one? e.g. "Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method'

+1 clearer titles and descriptions help a lot to ease the "cognitive burden".

jbosstm-bot · 2024-09-11T10:51:50Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/489/

jbosstm-bot · 2024-09-11T10:54:49Z

LRA profile tests failed (https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/489/): LRA Test failed with failures in no_arq profile

jbosstm-bot · 2024-09-11T10:55:35Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/489/

jbosstm-bot · 2024-09-11T11:19:12Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/489/

in LRAService joinLRA method

jbosstm-bot · 2024-09-11T14:04:21Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/494/

jbosstm-bot · 2024-09-11T14:04:23Z

Started testing this pull request with LRA profile: https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/494/

jbosstm-bot · 2024-09-11T14:07:13Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/494/

marcosgopen · 2024-09-11T14:21:12Z

LRA profile tests failed (https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK11,label=jnlp-agent/489/): LRA Test failed with failures in no_arq profile

I investigated the reason of the failure and, long story short, this is not related to this PR.
However I am working on that for another issue: https://issues.redhat.com/browse/JBTM-3922.

jbosstm-bot · 2024-09-11T14:27:59Z

LRA profile tests passed - Job complete https://ci-jenkins-csb-narayana.apps.ocp-c1.prod.psi.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=openJDK17,label=jnlp-agent/494/

marcosgopen requested a review from xstefank August 26, 2024 15:11

xstefank reviewed Aug 27, 2024

View reviewed changes

marcosgopen added the Hold label Aug 27, 2024

marcosgopen force-pushed the JBTM-3911 branch from ee76508 to 217fd5f Compare August 27, 2024 08:56

marcosgopen removed the Hold label Sep 2, 2024

marcosgopen requested a review from mmusgrov September 2, 2024 09:04

mmusgrov requested changes Sep 4, 2024

View reviewed changes

marcosgopen added the Hold label Sep 4, 2024

marcosgopen force-pushed the JBTM-3911 branch from 217fd5f to 25e40e5 Compare September 4, 2024 15:54

marcosgopen removed the Hold label Sep 4, 2024

marcosgopen force-pushed the JBTM-3911 branch from 25e40e5 to 7d2db7d Compare September 5, 2024 11:32

marcosgopen commented Sep 5, 2024

View reviewed changes

marcosgopen force-pushed the JBTM-3911 branch from 7d2db7d to 3905dc7 Compare September 5, 2024 13:39

marcosgopen force-pushed the JBTM-3911 branch from bb500ee to 888ab39 Compare September 10, 2024 10:09

marcosgopen commented Sep 10, 2024

View reviewed changes

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LongRunningAction.java Show resolved Hide resolved

mmusgrov requested changes Sep 10, 2024

View reviewed changes

marcosgopen force-pushed the JBTM-3911 branch from 888ab39 to ff5660c Compare September 11, 2024 09:49

mmusgrov reviewed Sep 11, 2024

View reviewed changes

mmusgrov approved these changes Sep 11, 2024

View reviewed changes

marcosgopen force-pushed the JBTM-3911 branch from ff5660c to 32f16a7 Compare September 11, 2024 13:20

marcosgopen changed the title ~~JBTM-3911 Remove redundant lock monitor from LRAService~~ JBTM-3911 Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method Sep 11, 2024

marcosgopen force-pushed the JBTM-3911 branch from 32f16a7 to d9dfdd2 Compare September 11, 2024 13:37

JBTM-3911 Replace synchronized in favor of Reentrant Lock

fd44514

in LRAService joinLRA method

marcosgopen force-pushed the JBTM-3911 branch from d9dfdd2 to fd44514 Compare September 11, 2024 13:54

marcosgopen merged commit eb77841 into jbosstm:main Sep 11, 2024

JBTM-3911 Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method #2293

JBTM-3911 Replace synchronized in favor of Reentrant Lock in LRAService joinLRA method #2293

Conversation

marcosgopen commented Aug 26, 2024 • edited Loading

jbosstm-bot commented Aug 26, 2024

jbosstm-bot commented Aug 26, 2024

jbosstm-bot commented Aug 26, 2024

jbosstm-bot commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbosstm-bot commented Aug 27, 2024

jbosstm-bot commented Aug 27, 2024

jbosstm-bot commented Aug 27, 2024

jbosstm-bot commented Aug 27, 2024

Choose a reason for hiding this comment

marcosgopen commented Sep 4, 2024

jbosstm-bot commented Sep 4, 2024

jbosstm-bot commented Sep 4, 2024

jbosstm-bot commented Sep 4, 2024

jbosstm-bot commented Sep 4, 2024

jbosstm-bot commented Sep 5, 2024

jbosstm-bot commented Sep 5, 2024

jbosstm-bot commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusgrov Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbosstm-bot commented Sep 5, 2024

jbosstm-bot commented Sep 10, 2024

jbosstm-bot commented Sep 10, 2024

jbosstm-bot commented Sep 10, 2024

jbosstm-bot commented Sep 10, 2024

jbosstm-bot commented Sep 10, 2024

jbosstm-bot commented Sep 10, 2024

Choose a reason for hiding this comment

mmusgrov commented Sep 11, 2024

marcosgopen commented Sep 11, 2024

mmusgrov left a comment

Choose a reason for hiding this comment

mmusgrov commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

marcosgopen commented Sep 11, 2024

jbosstm-bot commented Sep 11, 2024

marcosgopen commented Aug 26, 2024 •

edited

Loading

mmusgrov Sep 9, 2024 •

edited

Loading