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.

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

JBTM-3260 On publishing a new Narayana release there should be created Quarkus PR to update Narayana version #1569

Closed
wants to merge 2 commits into from

Conversation

mayankkunwar
Copy link
Contributor

@mayankkunwar mayankkunwar commented Mar 17, 2020

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

MAIN
!QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS !PERF NO_WIN !RTS !AS_TESTS !TOMCAT !JACOCO !LRA

@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/1192/

@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: 2031013.181800 vrs 2016314.666538 (0.000000%: no change)
com.arjuna.ats.jta.xa.performance.JTAStoreTests.jtaTest: 453031.979404 vrs 466436.968179 (0.000000%: no change)
com.hp.mwtests.ts.arjuna.performance.Performance1.twoPhaseTest: 1418827.672012 vrs 1504955.993373 (0.000000%: no change)
com.hp.mwtests.ts.arjuna.performance.Performance1.onePhaseTest: 4209615.945433 vrs 4272752.417047 (0.000000%: no change)
com.hp.mwtests.ts.arjuna.atomicaction.CheckedActionTest.testCheckedAction: 48646.575084 vrs 48226.543036 (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

@jbosstm-bot
Copy link

@tomjenkinson tomjenkinson requested review from mmusgrov and removed request for tomjenkinson March 17, 2020 14:21
Copy link
Contributor

@ochaloup ochaloup left a comment

Choose a reason for hiding this comment

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

Please, have you tried to run it locally? It's important as the CI won't catch any issue in the script. It will be found only during the release process.

I think we need to have something similar as for WFLY here:
Then there is this read -p question.
https://github.com/jbosstm/narayana/blob/5.10.4.Final/narayana-release-process.sh#L17
The quarkus issue is different as it's not created via https://issues.redhat.com but at quarkus github. Ie.g this is a bug here: https://github.com/quarkusio/quarkus/issues/new?assignees=&labels=kind%2Fbug&template=bug_report.md&title=
and we should add with label area/narayana and kind/enhancement

So, let's summarize what should be a nice to have as a result:

@@ -143,6 +138,33 @@ then
fi
cd ..

# Raising PR for quarkus to upgrade Narayana to current version.
QRKSISSUE = WFLYISSUE
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's good to go with the same issue name with Quarkus as it was for WFLY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using the same issue name because when narayana-release-process.sh is called, WFLYISSUE is coming as third argument, and I was not knowing from where, when and who calls this file. Otherwise I would have used the fourth argument for QRKSISSUE.

git fetch upstream;
git checkout -b ${QRKSISSUE}
git reset --hard upstream/master
cd /bom/runtime/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this won't work as you go with abs path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

locally this was working, but anyways it is always better to use full path, so I will change it as requested.

CURRENT_VERSION_IN_QRKS=`grep 'narayana.version>' pom.xml | cut -d \< -f 2|cut -d \> -f 2`
sed -i "s/narayana.version>$CURRENT_VERSION_IN_QRKS/narayana.version>$CURRENT/g" pom.xml
cd ../../
git add /bom/runtime/pom.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this won't work (abs path used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will refactor this as per request

git add /bom/runtime/pom.xml
git commit -m "${QRKSISSUE} Upgrade Narayana to $CURRENT"
git push --set-upstream jbosstm ${QRKSISSUE}
git checkout 5_BRANCH
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there is no 5_BRANCH in quarkus repo. I think the master should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will change it to master.

@ochaloup
Copy link
Contributor

@mayankkunwar ok, I'm putting this PR on hold as @xstefank pointed me out to the dependabot. Maybe we may just add the Narayana dependency to the quarkus config dependabot file (https://github.com/quarkusio/quarkus/blob/master/.dependabot/config.yml). Just I'm not sure about rules of such thing.

Please, do not proceed with the changes that I suggested and let's talk about the bot tomorrow - if it's a feasible option for us.

@ochaloup ochaloup added the Hold label Mar 17, 2020
@mayankkunwar
Copy link
Contributor Author

@ochaloup okay, we can have a discussion on the same before moving forward.

@mayankkunwar
Copy link
Contributor Author

I am going to close this PR, as we tried to solve this problem with a better alternative solution, by adding narayana dependencies to quarkus dependabot config.xml.
Here is the PR link: quarkusio/quarkus#7977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants