-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
WFLY-18383 Quickstart for MicroProfile LRA #778
Conversation
860e226
to
1c1dc3d
Compare
microprofile-lra/README.adoc
Outdated
link:https://github.com/eclipse/microprofile-lra[MicroProfile LRA specification] aims to provide an API that the | ||
applications utilize to cooperate actions in | ||
distributed | ||
transactions based on the saga pattern. The user applications enlist within the LRA whicquickstart_microprofile-openapi_ci.ymlh in turn notifies all enlisted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
within the LRA whicquickstart_microprofile-openapi_ci.ymlh
transactions based on the saga pattern. The user applications enlist within the LRA whicquickstart_microprofile-openapi_ci.ymlh in turn notifies all enlisted | |
transactions based on the saga pattern. The user applications enlist within the LRA which in turn notifies all enlisted |
---- | ||
+ | ||
|
||
[[solution]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is probably due to the line 121 +
include::../shared-doc/undeploy-the-quickstart.adoc[leveloffset=+1] | ||
|
||
// Restore the {productName} Standalone Server Configuration | ||
include::../shared-doc/restore-standalone-server-configuration.adoc[leveloffset=+1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, @xstefank please also add the following include, after that one:
// Restore the {productName} Standalone Server Configuration Manually
include::../shared-doc/restore-standalone-server-configuration-manual.adoc[leveloffset=+1]
---- | ||
<maven.compiler.source>11</maven.compiler.source> | ||
<maven.compiler.target>11</maven.compiler.target> | ||
---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no instructions regarding
<version.server>30.0.0.Final</version.server>
<version.bom.ee>${version.server}</version.bom.ee>
<version.bom.microprofile>${version.server}</version.bom.microprofile>
<version.plugin.wildfly>4.2.0.Final</version.plugin.wildfly>
<version.plugin.wildfly-jar>10.0.0.Final</version.plugin.wildfly-jar>
<version.cloud.fp>5.0.0.Final</version.cloud.fp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
PS: This is why I dislike "create from scratch" on readmes, those always have a hard time catching up with sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less work for us, more investigations for the user/customer. I still think it's worth having step by step instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we could also say copy the pom.xml from the QS but since the QS is not independent of the parent properties this is not possible.
|
||
NOTE: The `@Complete` and `@Compensate` methods don't need to be JAX-RS methods. See the specification for details. | ||
|
||
Now we are already able to start our first LRA. You can deploy the application to the {productName} as demonstrated in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we are not able. There is missing ParticipantResult.java code in the Readme
@@ -25,6 +25,11 @@ ifdef::reactive-messaging[] | |||
<layer>cloud-server</layer> | |||
<layer>h2-default-datasource</layer> | |||
<layer>microprofile-reactive-messaging-kafka</layer> | |||
endif::[] | |||
ifdef::microprofile-lra[] | |||
<layer>jaxrs-server</layer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would prefer to have here an abstract <layers>...<layers/>
, which suits all and needs no maintenance. @zhantemirov @xstefank WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't increase the server build time significantly, I'm OK with that change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually referring to the docs only, replace the layers config in the XML snippet with what I proposed above. This would be done on all asciidocs having the plugins config showing up layers XML, for both wildfly and wildfly-jar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no easy way to override this. Unfortunately for LRA and RM we need to have different layers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short, you can revert this and no worries with the layers config on READMEs, that should be gone soon.
Same for the changes for shared-doc/build-the-quickstart-for-openshift.adoc
<!-- The versions for the BOMs, Packs and Plugins --> | ||
<version.bom.ee>${version.server}</version.bom.ee> | ||
<version.bom.microprofile>${version.server}</version.bom.microprofile> | ||
<version.pack.cloud>5.0.0.Final</version.pack.cloud> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it to <version.cloud.fp>5.0.0.Final</version.cloud.fp> in order to match https://github.com/wildfly/quickstart/blob/main/shared-doc/build-the-quickstart-for-openshift.adoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be instead fixed in the build-the-quickstart-for-openshift.adoc , this is the new common property name used since the enhancements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmartins should be then the <version>${version.wildfly.maven.plugin}</version>
property dropped from the example openshift profile? It won't build once I copy it to my "create-from-scratch" project since we don't have this property defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that element is not needed since we do pluginManagement of the version. I guess that we need pluginmanagement documented too... Either that or get ridden of the create from scratch part, which imho provides little value, specially considering soon we will have a guide for developing apps from scratch.
@xstefank please consider adding some clauses to https://raw.githubusercontent.com/wildfly/quickstart/main/shared-doc/build-the-quickstart-for-openshift.adoc - the Readme instructions use wrong layers:
instead of:
|
microprofile-lra/README.adoc
Outdated
|
||
== Conclusion | ||
|
||
MicroProfile LRA provides a simple API for the distributed transactions based on the saga pattern. To use it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MicroProfile LRA provides a simple API for the distributed transactions based on the saga pattern. To use it | |
MicroProfile LRA provides a simple API for the distributed transactions based on the saga pattern. To use it on |
microprofile-lra/README.adoc
Outdated
|
||
MicroProfile LRA provides a simple API for the distributed transactions based on the saga pattern. To use it | ||
{productName} we need to enable the appropriate extensions and subsystems for the LRA Coordinator (a service that | ||
manages LRAs) and the LRA participant (client API). The LRAs are controlled trough annotations provided by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manages LRAs) and the LRA participant (client API). The LRAs are controlled trough annotations provided by the | |
manages LRAs) and the LRA participant (client API). The LRAs are controlled through annotations provided by the |
microprofile-lra/charts/heml.yaml
Outdated
@@ -0,0 +1,6 @@ | |||
build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this file to helm.yaml
@xstefank while creating whole MP-lRA QS from the scratch, I ran into an issue while trying to build the openshift profile (I have all necessary Java classes and pom.xml changes):
The profile configuration should be equal to this, but I didn't succeed to make it work. Could you please have a look? |
@xstefank please have a look at OpenShift testing section:
The https://$(oc get route microprofile-lra --template='{{ .spec.host }}') resolves correctly, the tests are failing even when I'm passing a valid https URL of OpenShift deployment. I also have an imported OpenShift cluster SSL certificate, so this shouldn't be a cause of the issue. |
1/ mvn clean package -Popenshift builds without issue. I don't know what could cause the error message you posted. 2/ I also verified on OpenShift sandbox mvn verify -Pintegration-testing -Dserver.host=https://$(oc get route microprofile-lra --template='{{ .spec.host }}') and it works all tests passing. |
85c1755
to
8e3214c
Compare
Apart from that, it LGTM |
1/ with the latest changes the version are there https://github.com/wildfly/quickstart/pull/778/files#diff-9473084512ad678e1593a36499d53db5eb7e5725ac6760364e4a139c1e67283aR188-R189 and https://github.com/wildfly/quickstart/pull/778/files#diff-a707e14145c2d812182a60503710696acb76495b0953320ad7add3834c865e78R38. So what's missing? Add build-the-quickstart-for-openshift.adoc I can remove ${version.wildfly.maven.plugin} but I would leave this to @emmartins and follow up. Overall, once you will have a guide to set up project, we should adjust all MP quickstart to include that and just change what they need so we can still keep instructions but leave out the project set up. |
@xstefank 1/ can you please send a screenshot, my browsers don't display me the lines you're trying to show. And it seems like you pasted the same link. I mean when you're writing an app from a scratch and trying to copy-paste the openshift profile from Readme instructions (not from existing QS pom.xml), the mvn clean package -Popenshift fails with an error due to the fact that the pom.xml of newly created app misses WFLY parent definition. But as I said previously, I'm not sure whether it's a valid use case - trying to copy-paste openshift profile from Readme section, not from existing QS pom.xml. I'll wait for @emmartins reply. |
@zhantemirov thanks, fixed the links. When you click the link, you need to manually open bigger number of changes files like README here, so just click load diff and you will be taken to the correct line. |
LGTM in terms of QS itself. Thank you, @xstefank @emmartins please note that we still need to resolve things related to OpenShift instructions in some follow-up PRs, you can find my findings at my previous comments. |
|
@emmartins @zhantemirov rebased, please take a look |
microprofile-lra/README.adoc
Outdated
INFO [org.wildfly.quickstarts.microprofile.lra.LRAParticipant2] (default task-5) Compensating action for Participant 2 (http://localhost:8080/lra-coordinator/lra-coordinator/recoveryhttp%3A%2F%2Flocalhost%3A8080%2Flra-coordinator%2Flra-coordinator%2F0_ffff0aca949a_-4998614b_64e74427_38b/0_ffff0aca949a_-4998614b_64e74427_38f) in LRA http://localhost:8080/lra-coordinator/lra-coordinator/0_ffff0aca949a_-4998614b_64e74427_38b. | ||
---- | ||
|
||
//Bootable JAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xstefank @zhantemirov this is optional but I believe a good change wrt user experience and our priorities... I think we should move this bootable jar section include, and the one after wrt to openshift, to https://github.com/wildfly/quickstart/pull/778/files#diff-9473084512ad678e1593a36499d53db5eb7e5725ac6760364e4a139c1e67283aR157 , ie to include those before the create-from-scratch instructions start. This change will put the bootable-jar and openshift at same level as baremetal, right now it kind of gives baremetal the spotlight, which is undesirable considering our priorities.
@emmartins @zhantemirov pushed the reordering |
thank you :-) |
https://issues.redhat.com/browse/WFLY-18383