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

Update to SmallRye Fault Tolerance 5.1.0 and add Vert.x integration #17328

Merged
merged 2 commits into from
May 24, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented May 18, 2021

Fixes #17080

@Ladicek
Copy link
Contributor Author

Ladicek commented May 18, 2021

Draft because

  • I want to let CI run in my fork;
  • I need to add some tests.

@famod
Copy link
Member

famod commented May 18, 2021

Is this also going to fix the Java 16 issues?

@Ladicek
Copy link
Contributor Author

Ladicek commented May 19, 2021

Indeed this will fix the Java 16 issues. I totally forgot to revert that commit -- thanks for reminder!

@Ladicek Ladicek force-pushed the fault-tolerance-vertx-integration branch 3 times, most recently from 9020019 to f17592d Compare May 21, 2021 15:09
@Ladicek Ladicek force-pushed the fault-tolerance-vertx-integration branch from f17592d to 490ca98 Compare May 21, 2021 18:00
@Ladicek
Copy link
Contributor Author

Ladicek commented May 21, 2021

CI passed in my fork.

@Ladicek Ladicek marked this pull request as ready for review May 21, 2021 21:15
@Ladicek
Copy link
Contributor Author

Ladicek commented May 21, 2021

@michalszynkiewicz could you please review the SmallRyeFaultToleranceProcessor change? (Sorry, can't think of anyone else, I don't really know who wrote the FT extension originally :-) )

@famod
Copy link
Member

famod commented May 22, 2021

Have you tried running the tcks tests locally with Java 16? Thing is CI runs them only with Java 11.
What I could do alternatively is to create a branch from yours here in the main repo and run the EA job for it...

@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2021

I did run the TCK locally with Java 16, but only on the SmallRye Fault Tolerance repo, that is, with Weld. Let me do that again with Quarkus.

@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2021

This is what I just did locally, on commit 490ca98, that is, on the branch corresponding to this PR:

find . -type d -name target -print0 | xargs -0 rm -rf
mvn clean install -Dquickly
export JAVA_HOME=/usr/lib/jvm/java-16-openjdk-amd64
cd tcks/microprofile-fault-tolerance/
mvn clean verify -Dtcks

And the result is Tests run: 435, Failures: 0, Errors: 0, Skipped: 0.

In other words, the MP FT TCK passes on Java 16 just fine :-)

@cescoffier
Copy link
Member

Should we merge this one?

@famod I believe @Ladicek did the Java 16 tests.

@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2021

Merging would certainly be nice :-)

@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2021

I mean, @cescoffier if you could take a look at what I did to the Vert.x extension, that would be even nicer! I couldn't think of a better place where to put the integration dependency. An unfortunate side effect is that the quarkus-vertx artifact now transitively brings exactly 1 new dependency, smallrye-fault-tolerance-vertx, but that thing only includes like 4 classes and weights 6 KBs, so should be fine.

@famod
Copy link
Member

famod commented May 24, 2021

Thanks for checking the FT TCK on Java 16!

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM, the new dependency is acceptable as it rather small. We can revisit this later if need be.

@cescoffier cescoffier merged commit 5fb0ddb into quarkusio:main May 24, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 24, 2021
@Ladicek Ladicek deleted the fault-tolerance-vertx-integration branch May 24, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable fault-tolerance tests on JDK 16+
3 participants