Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JBTM-3221 Refactor NarayanaLRARecovery according to the specification… #1531

Merged
merged 1 commit into from
Dec 3, 2019
Merged

JBTM-3221 Refactor NarayanaLRARecovery according to the specification… #1531

merged 1 commit into from
Dec 3, 2019

Conversation

xstefank
Copy link
Collaborator

@xstefank xstefank commented Nov 29, 2019 •
edited
Loading

… upgrades

https://issues.jboss.org/browse/JBTM-3221

!QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS !PERF NO_WIN !AS_TESTS !TOMCAT !JACOCO !mysql !postgres !db2 !oracle !RTS !MAIN

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

Started testing this pull request with PERF profile: http://172.17.130.4:8083/job/btny-pulls-narayana-perf/PROFILE=PERF,jdk=jdk8.latest,label=master/1116/

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

tomjenkinson
tomjenkinson reviewed Nov 29, 2019
View reviewed changes
...t-arquillian-extension/src/main/java/io/narayana/lra/arquillian/spi/NarayanaLRARecovery.java
System.getProperty(NarayanaLRAClient.LRA_COORDINATOR_HOST_KEY, "localhost"),
Integer.parseInt(System.getProperty(NarayanaLRAClient.LRA_COORDINATOR_PORT_KEY, "8080")));
public void waitForCallbacks(URI lraId) {
// no action is needed, tck callbacks calls are sufficiently fast for TCK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/service/spi/LRARecoveryService.java#L49 could have a default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that now with the PR #1530 we will need 1s waits (this replaces shortDelays). But of course, this can be added to the spec. Thanks for the pointer @tomjenkinson.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code base in this LRA branch does not currently need an implementation.
I am okay to resolve this when I/we merge the LRA branch into master - ie we should decide then whether or not a default or an implementation is required.

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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)
(changes within the % range [-10.000000, 10.000000] are regarded as insignificant):

com.hp.mwtests.ts.arjuna.atomicaction.CheckedActionTest.testThreadActionData: 2090955.786888 vrs 2127533.576171 (-1.719258%: no change)
com.arjuna.ats.jta.xa.performance.JTAStoreTests.jtaTest: 412315.090975 vrs 422052.002859 (-2.307041%: no change)
com.hp.mwtests.ts.arjuna.atomicaction.CheckedActionTest.testCheckedAction: 48413.915533 vrs 48041.440650 (0.775320%: no change)
com.hp.mwtests.ts.arjuna.performance.Performance1.twoPhase: 1418097.677394 vrs 1402818.396685 (1.089185%: no change)
com.hp.mwtests.ts.arjuna.performance.Performance1.onePhase: 4352016.073336 vrs 4689633.057388 (-7.199220%: 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

@jbosstm-bot
Copy link

PERF profile job finished http://172.17.130.4:8083/job/btny-pulls-narayana-perf/PROFILE=PERF,jdk=jdk8.latest,label=master/1116/

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

tomjenkinson
tomjenkinson approved these changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@tomjenkinson tomjenkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - @mmusgrov, please can you review too given it is in LRA?

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

@jbosstm-bot
Copy link

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

mmusgrov
mmusgrov approved these changes Dec 2, 2019
View reviewed changes
...t-arquillian-extension/src/main/java/io/narayana/lra/arquillian/spi/NarayanaLRARecovery.java
System.getProperty(NarayanaLRAClient.LRA_COORDINATOR_HOST_KEY, "localhost"),
Integer.parseInt(System.getProperty(NarayanaLRAClient.LRA_COORDINATOR_PORT_KEY, "8080")));
public void waitForCallbacks(URI lraId) {
// no action is needed, tck callbacks calls are sufficiently fast for TCK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code base in this LRA branch does not currently need an implementation.
I am okay to resolve this when I/we merge the LRA branch into master - ie we should decide then whether or not a default or an implementation is required.

...t-arquillian-extension/src/main/java/io/narayana/lra/arquillian/spi/NarayanaLRARecovery.java
return false;
}

if (json.contains(lraId.toASCIIString())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is up to you but why not just:

    return !json.contains(lraId.toASCIIString())

@mmusgrov
Copy link
Contributor

mmusgrov commented Dec 3, 2019

I will merge this now. Thanks for the contribution.

@mmusgrov mmusgrov merged commit 07d847c into jbosstm:LRA Dec 3, 2019
@xstefank
Copy link
Collaborator Author

xstefank commented Dec 3, 2019

ah sorry, I haven't got to this in time. I will make the suggested change in my next PR if you don't mind.

@xstefank xstefank deleted the lra-issue-249 branch December 4, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomjenkinson tomjenkinson tomjenkinson approved these changes

@mmusgrov mmusgrov mmusgrov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@xstefank @jbosstm-bot @mmusgrov @tomjenkinson