[JBTM-3381] Enhance LRA Client for Quarkus #1707

ruromero · 2020-10-29T11:47:56Z

Hey,

I wanted to do a small contribution to the client after starting to use it. I hope you find my changes useful and relevant.

Clean up unnecessary dependencies and reformat pom.
Set PORT and PATH_KEY as constants. I couldn't import them properly in my code
Fix the clientID param that was sending an object string representation.
joinLRA with a null timelimit was throwing a NPE during the unboxing.
Other SonarLint suggestions.
The service provided has been replaced by the Microprofile implementation.

I didn't consider meaningful creating a Jira for this but if you feel it's a requirement even for such a small PR I'll do it.
Let me know if I didn't take something into account, specially regarding the pom.xml.

Thank you in advance.

Signed-off-by: ruromero rromerom@redhat.com

Fix https://issues.redhat.com/browse/JBTM-3381

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 · 2020-10-29T11:50:12Z

⚠️ narayana CI not started.

Author is not the 'narayana' contributor, to permit PR being run members of jbosstm can write comment of text: TESTIT

ochaloup · 2020-10-29T12:12:46Z

TESTIT

jbosstm-bot · 2020-10-29T12:14:46Z

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

jbosstm-bot · 2020-10-29T13:14:58Z

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

mmusgrov · 2020-10-29T14:32:41Z

@ruromero Thanks, the changes look good except for some procedural tasks:

if you haven't already signed our CLA please will you do that;
changes must be relevant to the issue being fixed so we don't normally accept code formatting changes;
we also prefer that any PR which updates the code has a corresponding JIRA issue (this helps with tracking changes and for generating release notes);

ochaloup

I have few minor points about the code changes. Otherwise it's nice. Thanks.

rts/lra/client/src/main/java/io/narayana/lra/client/NarayanaLRAClient.java

ochaloup · 2020-10-29T19:39:05Z

rts/lra/client/src/main/java/io/narayana/lra/client/NarayanaLRAClient.java

@@ -320,13 +321,6 @@ public URI startLRA(URI parentLRA, String clientID, Long timeout, ChronoUnit uni
            }

            lra = URI.create(response.getHeaderString(HttpHeaders.LOCATION));
-
-            if (lra == null) {
-                LRALogger.i18NLogger.error_nullLraOnCreation(response.toString());


Please with this remove the method error_nullLraOnCreation from the i18NLogger as well.

rts/lra/client/src/main/java/io/narayana/lra/client/NarayanaLRAClient.java

ochaloup · 2020-10-29T19:44:09Z

rts/lra/client/src/main/resources/META-INF/services/javax.ws.rs.client.ClientBuilder

@@ -1 +1 @@
-org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder
+org.jboss.resteasy.microprofile.client.impl.MpClientBuilderImpl


With this change we should be consistent in whole LRA impl. The same change should be done in https://github.com/jbosstm/narayana/blob/master/rts/lra/jaxrs/src/main/resources/META-INF/services/javax.ws.rs.client.ClientBuilder.
I think especially this particular change would be nice to be tracked as a jira.
FYI @xstefank

If you agree on the change I will create the JIRA for this and move it to a specific PR.

ruromero · 2020-10-29T20:59:07Z

@mmusgrov

The link you provided to the CLA doesn't seem to work. I have also tried to follow cla.jboss.org but I'm being redirected to a RH site.
I will remove code formatting specific changes.
Finally, as I was submitting changes so different that I found during the LRA client integration into my project, I didn't know what JIRA to create because first I wanted to know if they made sense at all. Please, tell me what JIRA or JIRAs should I create for each specific change.

Thanks.

jbosstm-bot · 2020-10-29T21:05:48Z

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

jbosstm-bot · 2020-10-29T21:59:09Z

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

jbosstm-bot · 2020-10-30T10:11:01Z

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

ruromero · 2020-10-30T10:11:42Z

@ochaloup @mmusgrov I have created the Jira and updated the PR.
I have also removed a dependency that was preventing me from running the client in a Quarkus native app.

jbosstm-bot · 2020-10-30T11:06:49Z

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

ochaloup

Seems fine to me. Thanks.

mmusgrov · 2020-10-30T11:28:11Z

@mmusgrov

The link you provided to the CLA doesn't seem to work. I have also tried to follow cla.jboss.org but I'm being redirected to a RH site.

The revised process for contributions is to include "Signed-off-by:" followed by the contributor’s name and email address in the commit message (which you already did):

Signed-off-by: Your Name <yourname@example.com>

With git, this step can be automated using git commit -s or git commit --signoff.

I will update the automated text generated in new PRs to include this information.

Thanks for bringing this policy change to our attention.

mmusgrov · 2020-10-30T12:47:06Z

rts/lra/client/src/main/java/io/narayana/lra/client/NarayanaLRAClient.java

@@ -305,7 +305,7 @@ public URI startLRA(URI parentLRA, String clientID, Long timeout, ChronoUnit uni

            response = client.target(base)
                .path(START_PATH)
-                .queryParam(CLIENT_ID_PARAM_NAME, client)
+                .queryParam(CLIENT_ID_PARAM_NAME, URLEncoder.encode(clientID, "UTF-8"))


Is it wise to encode the clientID? A few of points:

it is not necessarily a URL since client ids are arbitrary strings and it may be confusing to the client to receive back something other that what he set on the start call;

the json data returned by the API call for returning LRAs and the format of the returned data doesn't say anything about us encoding the client id

encoded urls are usually for use in html pages but the client id is not for this purpose.

Encoding of the clientID was my idea. I was afraid that the URL escaped characters won't be correctly transfered and interpreted. I was wrong as I can see from testing the functionality.
Agree that encoding is not necessary, sorry for misguidance @ruromero

No worries, I have reverted the change.

mmusgrov

There is just my issue about encoding the clientID remaining.

Signed-off-by: ruromero <rromerom@redhat.com>

ruromero · 2020-10-30T16:55:17Z

Done @mmusgrov

jbosstm-bot · 2020-10-30T16:57:27Z

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

jbosstm-bot · 2020-10-30T18:25:20Z

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

ochaloup suggested changes Oct 29, 2020

View reviewed changes

ruromero requested a review from ochaloup October 29, 2020 21:04

ruromero force-pushed the client branch from 7435f52 to 8ae1a6a Compare October 30, 2020 10:09

ruromero changed the title ~~small updates to client~~ [JBTM-3381] Enhance LRA Client for Quarkus Oct 30, 2020

ochaloup approved these changes Oct 30, 2020

View reviewed changes

mmusgrov reviewed Oct 30, 2020

View reviewed changes

mmusgrov requested changes Oct 30, 2020

View reviewed changes

[JBTM-3381] Enhance Quarkus compatibility

50f97bb

Signed-off-by: ruromero <rromerom@redhat.com>

ruromero force-pushed the client branch from 8ae1a6a to 50f97bb Compare October 30, 2020 16:54

mmusgrov approved these changes Oct 31, 2020

View reviewed changes

mmusgrov merged commit 55e4b71 into jbosstm:master Oct 31, 2020

ruromero deleted the client branch November 3, 2020 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JBTM-3381] Enhance LRA Client for Quarkus #1707

[JBTM-3381] Enhance LRA Client for Quarkus #1707

ruromero commented Oct 29, 2020 •

edited

jbosstm-bot commented Oct 29, 2020

ochaloup commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

mmusgrov commented Oct 29, 2020

ochaloup left a comment

ochaloup Oct 29, 2020

ochaloup Oct 29, 2020

ruromero Oct 29, 2020

ruromero commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 30, 2020

ruromero commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

ochaloup left a comment

mmusgrov commented Oct 30, 2020 •

edited

mmusgrov Oct 30, 2020 •

edited

ochaloup Oct 30, 2020

ruromero Oct 30, 2020

mmusgrov left a comment

ruromero commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

		@@ -1 +1 @@
		org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder
		org.jboss.resteasy.microprofile.client.impl.MpClientBuilderImpl

[JBTM-3381] Enhance LRA Client for Quarkus #1707

[JBTM-3381] Enhance LRA Client for Quarkus #1707

Conversation

ruromero commented Oct 29, 2020 • edited

jbosstm-bot commented Oct 29, 2020

ochaloup commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

mmusgrov commented Oct 29, 2020

ochaloup left a comment

Choose a reason for hiding this comment

ochaloup Oct 29, 2020

Choose a reason for hiding this comment

ochaloup Oct 29, 2020

Choose a reason for hiding this comment

ruromero Oct 29, 2020

Choose a reason for hiding this comment

ruromero commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 29, 2020

jbosstm-bot commented Oct 30, 2020

ruromero commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

ochaloup left a comment

Choose a reason for hiding this comment

mmusgrov commented Oct 30, 2020 • edited

mmusgrov Oct 30, 2020 • edited

Choose a reason for hiding this comment

ochaloup Oct 30, 2020

Choose a reason for hiding this comment

ruromero Oct 30, 2020

Choose a reason for hiding this comment

mmusgrov left a comment

Choose a reason for hiding this comment

ruromero commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

jbosstm-bot commented Oct 30, 2020

ruromero commented Oct 29, 2020 •

edited

mmusgrov commented Oct 30, 2020 •

edited

mmusgrov Oct 30, 2020 •

edited