[JBTM-3294] adding HTTP version headers for coordinator #1783

ochaloup · 2021-02-10T13:06:12Z

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

I'm publishing this withouth having finished similar work which I would like to do with RecoveryCoordinator. I hope for Coordinator this is acceptable to be discussed and hopefully merged.

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

/cc @xstefank

jbosstm-bot · 2021-02-10T13:10:50Z

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

jbosstm-bot · 2021-02-10T15:13:10Z

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/84/): LRA Test failed

jbosstm-bot · 2021-02-10T17:35:47Z

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

jbosstm-bot · 2021-02-10T19:38:08Z

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/85/): LRA Test failed

xstefank · 2021-02-10T19:34:08Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java


-        return Response.status(status).build();
+    private void verifyVersion(String versionString) {


This seems it should be encapsulated in the APIVersion class and reused for the client in my opinion.

And do you mean to move the APIVersion under service-base module, right?
That could be an option. I was thiking the APIVersion is rather internal for the coordinator. How that could be reused for the client?

Client is not going to also verify the version of the server?

+1, I will address your point. Thanks.

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/JaxRsActivator.java

xstefank · 2021-02-10T19:37:39Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/JaxRsActivator.java

+     * Any bigger version is considered as unimplemented and unknown.
+     * Any lower version is considered as older, implemented but deprecated and in case not supported.
+     */
+    public static final String LRA_API_VERSION_STRING = "1.0-RC1";


I would strongly suggest not tightening Narayana API to LRA API in this way. We can maybe in the future want to change our API async from the LRA releases.

This was not an intention in any way. But marking the initial API with version 1.0 sounds me reasonable.
Would be more understandable for you if I change the variable name to NARAYANA_LRA_API_VERSION_STRING (+ changing the comment)? Or do you suggest some different way of approaching this?

yes, to both points. Make it clear this is narayana specific and not tight to MP LRA in any way. Also why not use also micros? More flexibility for us to do updates in the future? So the first one should be IMO 1.0.0.

I'm really for (at most) major.minor.
I was inspired from other APIs and there are either v1 (e.g. github) or major.minor (eg. facebook, M$) or timestamps (twillio). There are even APIs that does not support versioning as they consider it non-RESTful. A bit what I summarized is here: https://docs.google.com/document/d/1ecyNgFY-ywTSA7uVH7MhhkhaooEQ5tF-daHkYtOoKiw/edit#
If there isn't some good reason/example where it's used the micro versions I would go with major.minor.

Why do we need the suffix -RC1. If 1.0 hasn't been released yet then just drop the suffix - the user will know he is using unreleased software so adding the suffix makes the code (on both the client and server ends) and policy more complex.

Ok, I missed this point. It makes sense and I will remove the -RC1 handling code then.

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/internal/APIVersion.java

rts/lra/service-base/src/main/java/io/narayana/lra/LRAConstants.java

rts/lra/service-base/src/main/java/io/narayana/lra/logging/lraI18NLogger.java

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

jbosstm-bot · 2021-02-11T12:20: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/87/

jbosstm-bot · 2021-02-11T14:17:59Z

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

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/internal/APIVersion.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java

rts/lra/pom.xml

rts/lra/service-base/src/main/java/io/narayana/lra/LRAData.java

rts/lra/service-base/src/main/java/io/narayana/lra/logging/lraI18NLogger.java

mmusgrov · 2021-02-11T19:03:57Z

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

+ */
+@RunWith(Arquillian.class)
+@RunAsClient
+public class CoordinatorApi_1_0_IT {


Can we have a test for incompatible versions?

The tests for incompatible versions are placed at the end of the test file. Is that enough or do you consider something more?

It is still not clear what constitutes an incompatible version, the 3 versions rule is somewhat arbitrary.
How are those tests determining incompatibility?

If the client asks for version e.g. 3.0 then the coordinator replies either "I don't know that version" or "responds with the content that is implemented for version 3.0. This test consider a client asking for version 1.0, ie. when client asks for 1.0 will it get the right content? When somebody changes the code in future then 1.0 has to still be returning what was returning. If the code change will start returning something else the tests start to blame.

I did not understand your response.

I did not understand your question.
I understand it that you're asking how these tests finds incompatibility with version 3.0. They are not addressing it.
These tests address the compatibility with 1.0. The implementation has to be still working with 1.0 even when a new version is released. If the implementation works with 1.0 - which these tests ensures - we are still fine and compatible.

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

ochaloup

Thanks for the review. I tried to address all the comments or I added my perspective.
Those which I considered resolved I marked as 'resolved', if you think differently please just re-open.
I'm now pushing the version without rebasing on top of the current master. I need to do it in the next step.

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java

rts/lra/service-base/src/main/java/io/narayana/lra/LRAData.java

rts/lra/service-base/src/main/java/io/narayana/lra/logging/lraI18NLogger.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/internal/APIVersion.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LRARecord.java

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java

ochaloup · 2021-02-13T10:18:56Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java

+        APIVersion apiVersion = null;
+        try {
+            apiVersion = APIVersion.instanceOf(versionString);
+            if (apiVersion.compareTo(currentAPIVersion) > 0) {


Yes, you're right that it could be good to have some guideline about this. The API compatibility is verified by the CoordinatorAPI_IT test which checks if the API is working with 1.0 version.
When a change is done then a new test case should be created under a new test file. The way how the code determines the version could be various. I was discussing that when we talk about API and some summary is at https://docs.google.com/document/d/1ecyNgFY-ywTSA7uVH7MhhkhaooEQ5tF-daHkYtOoKiw/edit

I think it could be good to have the ideas documented somewhere in the repo. Do you think such poits belong to README of the lra module? Or maybe creating an API.adoc that will be linked from the README?

For now I added the API.adoc document for this being documented.

jbosstm-bot · 2021-02-14T09:01:01Z

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/91/): Narayana rebase on master failed. Please rebase it manually

jbosstm-bot · 2021-02-14T14:25:58Z

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/92/): Narayana rebase on master failed. Please rebase it manually

ochaloup · 2021-02-15T10:44:28Z

@mmusgrov @xstefank Finally I managed to rebase against the master branch. The tests passed on my local machine, I hope it will pass on CI as well.
I consider this ready for review as it is now.

jbosstm-bot · 2021-02-15T10:45:45Z

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

jbosstm-bot · 2021-02-15T12:50:55Z

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

mmusgrov

I am trying to grasp the implications of the scheme you have introduced:

We might want to maintain an old version for a valued customer.
It breaks the content negotiation pillar of REST principles.
The version header is now hidden data and not discoverable which many REST APIs do support.
The 3 version rule is arbitrary and not normal for REST APIs.
The version on the request path needs to permit the caller to announce that it can supports multiple versions. The one selected by the implementation would then be indicated on the response path. This comment is referring to standard REST content negotiation where the caller indicates what kind of response it is capable of parsing.

mmusgrov · 2021-02-16T10:33:13Z

rts/lra/API.adoc

+== API definition as Open API document
+
+The Narayana LRA API is documented with Open API annotations at the java
+classes. The Open API definition needs to be published at the http://narayana.io


I will reference it in my PR for JBTM-2929 (Create LRA documentation)

I didn't mention API versioning in my community docs update, sorry

mmusgrov · 2021-02-16T10:35:36Z

rts/lra/API.adoc

+which demands (via HTTP header `Narayana-LRA-API-version`) version
+higher than the current API version (ie. the highest version that the API
+is created for).
+The unsupported version could be one that is already deprecated


So this is the "support for at least two previous major versions" rule.

Yes, and we can rephrase. If you have idea to say it better I'm glad to change it.

We have already discussed supporting multiple versions. An "unsupported version is one that is not supported", which sounds empty of content, but if you indicate how the user can discover which version(s) are supported then it has meaning. You could then add something like "typically the last 3 versions are supported but older version may also be maintained".

You can indicate how the user can discover supported versions with text similar to (I am just showing this as an example of the kind of text that is unambiguous):

Multiple versions of the API may be supported in parallel. The current version can be discovered by:

either looking at xyz (API.adoc or narayana.io website or wherever)

and/or by sending a request without the version header. The response will include the version header > containing the latest supported version

To discover which versions are supported send request with a header that asks for an invalid version
(recall that you have specified valid formats for versions so the user knows how to request an invalid
format).

Right, agree. I took your text just the last part seems to me duplicate to the items above. I didn't include it to PR. If that was wrong understanding, let me know I will change the text.

mmusgrov · 2021-02-16T10:37:03Z

rts/lra/API.adoc

+But for the start there is expected similar pattern like following
+
+NOTE: The verification of the highest supported API version could
+      be part of the `ContainerRequestFilter`/`ContainerResponseFilter`.


How we implement it is private to our implementation whereas this API.adoc is user facing.

The API.adoc was meant to be at the boundary of the developer and the outside world. It talks about code and best practices.
I can remove the code part and some points on internals. Would you prefer so?
I would probably move that text to wiki if it's fine with you.

I mean why does the user need to know that the coordinator is using filters to validate the version header?

I meant that the document was meant to be read by Narayana developers as well. But I removed the part about code changes. Hopefully now it's related only for API users.

rts/lra/API.adoc

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

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/Coordinator.java

mmusgrov · 2021-02-16T10:54:31Z

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

+ * This class is not expected to be changed. The Narayana LRA API of version 1.0 is not expected to be changed.<br/>
+ * When Coordinator changes the API there will be updated the API version for such change.
+ * The new or changed behaviour belongs to a separate test class
+ * which verifies the behaviour of the particular API version.


And when a version is no longer supported will we remove the associated test class?
And are most incompatibilities addition of functionality. How is that handled?

Yes, if the version is no longer supported then there is no reason having a test class which would be failing.
Incompatibilities could be rather changes in functionality, providing different enpoints, changing type of result etc. With such thing we need a new version and a new way of handling. And new test for such functionality.

I was thinking more about the situation where we add functionality then it seems overkill to have to write a new test for it. Is inheritance and option?

No, it's not overkill. The API should be working as it is for 1.0 it's what the client expects to be working. My thinking is that if there is not a bug in test or implementation of 1.0 this should be reflecting the state of 1.0.
The inheritance is an option. Then when a new functionality is added then there could be tested that functionality not to be copied all from here. But it depends.

mmusgrov · 2021-02-16T12:46:05Z

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

+ */
+@RunWith(Arquillian.class)
+@RunAsClient
+public class CoordinatorApi_1_0_IT {


It is still not clear what constitutes an incompatible version, the 3 versions rule is somewhat arbitrary.
How are those tests determining incompatibility?

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

mmusgrov · 2021-02-16T16:48:42Z

You closed conversation #1783 (comment) prematurely. The recursion happens on the line where I made the comment ie return JaxRsActivator.LRA_API_VERSION;

mmusgrov · 2021-02-16T17:09:35Z

I am trying to grasp the implications of the scheme you have introduced:

We might want to maintain an old version for a valued customer.

It breaks the content negotiation pillar of REST principles.

The version header is now hidden data and not discoverable which many REST APIs do support.

The 3 version rule is arbitrary and not normal for REST APIs.

The version on the request path needs to permit the caller to announce that it can supports multiple versions. The one selected by the implementation would then be indicated on the response path. This comment is referring to standard REST content negotiation where the caller indicates what kind of response it is capable of parsing.

We can address 1 and 4 by maintaining a list of supported versions (in APIVersion.java or elsewhere) and deprecate using @deprecated.
We can address 2 and 5 by specifying in API.adoc that the user can provide multiple Narayana-LRA-API-version headers to indicate which versions the caller can handle. And the one chosen by the implementation should be provided (via the header) in the response.
Comment 3 is invalid.

mmusgrov · 2021-02-16T17:12:39Z

We need documentation for each supported version. Is the OpenAPI document sufficient for this purpose?

jbosstm-bot · 2021-02-16T19:25:50Z

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

jbosstm-bot · 2021-02-16T20:00:47Z

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

ochaloup · 2021-02-16T20:01:20Z

@mmusgrov

ad You closed conversation #1783 (comment) prematurely. The recursion happens on the line where I made the comment ie return JaxRsActivator.LRA_API_VERSION;) Do you mean a potential recursion? I mean the instanceOf() is called on non-null non-empty instance. You mean the recursion may happen if somebody defines the LRAConstants.NARAYANA_LRA_API_VERSION_STRING to be null?

ad by maintaining a list of supported versions) sure, that has to be done. The supported versions for the release have to be documented.
I was not thinking about it to do for 1.0 but it's good point for that to be done. I changed the LRAConstants to contain the comment, anticipating there will be list of the supported versions. The way how this is done can be adjusted.

ad) the user can provide multiple Narayana-LRA-API-version headers to indicate which versions the caller can handle I don't know about any REST API implementation that would be support multiple version negotiation but yes, that could be implemented. Would be fine if I create a JIRA for it? For now when we have only one version there is not a rush for it, I think.

ad) I found one thing which I'm now unsure. Now we have the JaxRsActivator and then the Coordinator and the RecoveryCoordinator. All of them extends the Application class. Should not be the JaxRsActivator removed?

ad We need documentation for each supported version. Is the OpenAPI document sufficient for this purpose?) The OpenAPI purpose is to document the API endpoint. There could be described any information we need. The information about versioning or something else. If needed we can add some parameter or header which will add some more semantics if needed message the client directly.

jbosstm-bot · 2021-02-16T21:16:47Z

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

jbosstm-bot · 2021-02-16T21:52:32Z

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

ochaloup

I pushed minor code changes based on the review.

ochaloup · 2021-02-19T14:07:28Z

rts/lra/API.adoc

+
+The Narayana LRA REST API is expected to support for at least two previous
+`major` versions (ie. support is expected in parallel at least of versions 1.x,
+2.x and 3.x until the 4.0 is released).


I understand it. My perspective would be probably to go with 1 version backwards in parallel instead of 2 but on the other hand @mmusgrov has valid points to maybe prolong the support even longer.
There are different perspectives. Maybe the requirements could be adjusted based on changes happening in the code and the feedback from the community.

rts/lra/API.adoc

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

ochaloup · 2021-02-19T14:20:31Z

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/api/JaxRsActivator.java

+                description = "API version string in format [major].[minor]-[prerelease]. Major and minor are required and to be numbers, prerelease part is optional.")
+        },
+        headers = {
+            @Header(name = LRAConstants.LRA_API_VERSION_HEADER_NAME, description = "Narayana LRA API version that processed the request")


This is used only for reference (in Coordinator it's the 'ref = ...) - the components from @Infoare not being shown directly by openapi (as far as I can say). The PR uses the reference from both from@Headerand from@Parameter`. So I needed to declare it for both.

rts/lra/coordinator/src/main/java/io/narayana/lra/coordinator/domain/service/LRAService.java

ochaloup · 2021-02-19T14:26:10Z

rts/lra/service-base/src/main/java/io/narayana/lra/APIVersion.java

+            } else if (preRelease != null && anotherVersion.preRelease == null) {
+                result = -1;
+            } else {
+                result = preRelease.compareTo(anotherVersion.preRelease);


Yes, I made the string comparison intentional this way as the preRelease is definitive for the release and it's good for proposals. There could be arbitrary string which makes sense for the proposal and for its verification. What's important is the major, minor.

@mmusgrov I made a comment previously checking for unofficial versions that was never addressed I'm sorry I had to miss your question. May you, please, repeat your arguments for that? (I mean why the preRelease is unnecessary?)

ochaloup · 2021-02-19T14:30:26Z

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

+ */
+@RunWith(Arquillian.class)
+@RunAsClient
+public class CoordinatorApi_1_0_IT {


I did not understand your question.
I understand it that you're asking how these tests finds incompatibility with version 3.0. They are not addressing it.
These tests address the compatibility with 1.0. The implementation has to be still working with 1.0 even when a new version is released. If the implementation works with 1.0 - which these tests ensures - we are still fine and compatible.

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApi_1_0_IT.java

jbosstm-bot · 2021-02-19T14:46: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/106/

jbosstm-bot · 2021-02-19T14:50:47Z

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

jbosstm-bot · 2021-02-19T16:39:13Z

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

jbosstm-bot · 2021-02-19T16:41:10Z

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

… LRA pom.xml

jbosstm-bot · 2021-03-02T07:00:55Z

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

ochaloup · 2021-03-02T08:55:33Z

@mmusgrov per our discussion I tried to address all the points in your comment #1783 (comment)
I decided to use the parametrized test for the difficulty with the API test execution. I experimented with inheritance and I didn't like it. Please take a look what are your thoughts.
Thank you for your feedback.

jbosstm-bot · 2021-03-02T09:04:08Z

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

mmusgrov

I put my comments inline. I also was unable to review the tests (CoordinatorApiIT.java, CoordinatorApiWrongVersionIT.java , ValidTestVersions.java, ValidTestVersionsRule.java), sorry. I found them too complicated. Perhaps we could get one of the other team members to look at those (ie Manuel or Mayank).

mmusgrov · 2021-03-02T10:15:57Z

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/api/CoordinatorContainerFilter.java

+
+        String apiVersionString = requestContext.getHeaderString(LRAConstants.LRA_API_VERSION_HEADER_NAME);
+
+        // request contains multiple API version headers, using first one and printing a warning


Specifying multiple versions can be a way for the client to indicate that it willing to process multiple different versions/formats. This means it is not a user error and there should be no logging of the situation. In fact the ascii doc is explicit here, quote:

If the caller is able to accept more than one version then the caller should include the header for each one.

Ok. I would like to leave the logging for the investigation purposes if you are not strictly against such approach. Moving the log level to debug and changing the message.

mmusgrov · 2021-03-02T10:17:00Z

rts/lra/API.adoc

+== API definition as Open API document
+
+The Narayana LRA API is documented with Open API annotations at the java
+classes. The Open API definition needs to be published at the http://narayana.io


I didn't mention API versioning in my community docs update, sorry

mmusgrov · 2021-03-02T10:21:36Z

rts/lra/API.adoc

+in Narayana LRA coordinator service.
+This document is written for developer of Narayana LRA services.
+
+The version format is `major.minor`[`-preRelease`].


You said you were going to remove the pre release handling code (#1783 (comment))

There are also other comments still left unaddressed from the first review round. I will comment below on the API.adoc and on the version header testsuite since I know that is where we had most discussion, ie I am not going to do an exhaustive review until I know that the comments from the first round have been discussed and/or resolved.

Sorry, this is my omission. The text about the preRelease should be removed. The required is major.minor.

mmusgrov · 2021-03-02T10:32:43Z

rts/lra/API.adoc

+
+The Narayana LRA REST API is expected to support for at least two previous
+`major` versions (ie. support is expected in parallel at least of versions 1.x,
+2.x and 3.x until the 4.0 is released).


But you still haven't said that versions other than the last three may be supported.
Also how does the client discover which versions are supported? The natural place would have been to add it to the API.adoc section:

== Unsupported version errors

where multiple version headers are set in the response indicate which versions are supported. I have also added related comments below in section covering API.adoc.

mmusgrov · 2021-03-02T11:00:44Z

rts/lra/API.adoc

+which demands (via HTTP header `Narayana-LRA-API-version`) version
+higher than the current API version (ie. the highest version that the API
+is created for).
+The unsupported version could be one that is already deprecated


We have already discussed supporting multiple versions. An "unsupported version is one that is not supported", which sounds empty of content, but if you indicate how the user can discover which version(s) are supported then it has meaning. You could then add something like "typically the last 3 versions are supported but older version may also be maintained".

You can indicate how the user can discover supported versions with text similar to (I am just showing this as an example of the kind of text that is unambiguous):

Multiple versions of the API may be supported in parallel. The current version can be discovered by:

either looking at xyz (API.adoc or narayana.io website or wherever)

and/or by sending a request without the version header. The response will include the version header > containing the latest supported version

To discover which versions are supported send request with a header that asks for an invalid version
(recall that you have specified valid formats for versions so the user knows how to request an invalid
format).

mmusgrov · 2021-03-02T12:00:26Z

rts/lra/API.adoc

+      the same version with `-preRelease` part.
+
+Client may demand behaviour based on particular API version
+by providing HTTP header link:./service-base/src/main/java/io/narayana/lra/LRAConstants.java[`Narayana-LRA-API-version`] on the call.


You make the assumption that the value in the version header must exactly match the supported version, ie white space is significant. I guess that requirement is implicit?

Yes. But we can state it in the document. Or if you think the behaviour should be "less" strict.
Some text suggestion would be great in such case.

mmusgrov · 2021-03-02T12:04:30Z

rts/lra/service-base/src/main/java/io/narayana/lra/LRAConstants.java

+
+    /**
+     * The Narayana API version for LRA coordinator supported for the release.
+     * Any higher version is considered as unimplemented and unknown.


Maybe also remark that valid version formats are defined in API.adoc

I added that note to the prior javadoc section.

mmusgrov · 2021-03-02T12:05:31Z

rts/lra/service-base/src/main/java/io/narayana/lra/logging/lraI18NLogger.java

            "Reason: '%s'")
    void warn_move_lra_record(String failedUid, String exceptionMessage);

+    @LogMessage(level = WARN)
+    @Message(id = 25029, value = "Multiple version headers '%s' were provided for the request '%s'. " +
+            "Consider to provide only one such header, there will be used value '%s' for now.")


See my earlier comment.

I removed this.

mmusgrov · 2021-03-02T12:10:53Z

...tension/src/main/java/io/narayana/lra/arquillian/AppServerCoordinatorDeploymentObserver.java

@@ -53,15 +53,15 @@
     * which is deployed when the app server starts.
     */
    public void handleAfterStartup(@Observes AfterStart event, Container container) throws Exception {
-        if(!container.getName().contains(CONTAINER_NAME_RECOGNITION)) {
+        if (!container.getName().contains(CONTAINER_NAME_RECOGNITION)) {


It is clear that String LRA_COORDINATOR_DEPLOYMENT_NAME = "lra-coordinator"; is built into the deployment.
But it is not clear how String CONTAINER_NAME_RECOGNITION = "as-coordinator"; is set and used (for example it is not present in arquillian-wildfly.xml).

There was a bit of text in the definition of the field. I've extended it a little bit now.

mmusgrov · 2021-03-02T12:13:25Z

...st/arquillian-extension/src/main/java/io/narayana/lra/arquillian/ArquillianParametrized.java

+import java.util.List;
+
+/**
+ * Parameterized Arquillian JUnit runner adding the possibility to run parameterized JUnit test cases ,


"parameterized JUnit" test cases sounds good but what are they?
I guess it is something to do with simplifying the version conformance test suite, maybe.
It is not at all clear what the purpose of this class is.

The parametrized JUnit tests are tests which runs several times with different parameters - e.g. something here https://www.tutorialspoint.com/junit/junit_parameterized_test.htm
This is an adjustment of the Arquillian runner to be capable to run in the similar manner. It's for a generic use. The point is to use it for versions right now but it can be used in different tests as well.

+1

I like that you introduced parameters in the Arquillian runner. Good idea :-)

ochaloup

@mmusgrov I tried to address your inline comments. When you have time, please, take a look.

ochaloup · 2021-03-02T12:50:52Z

...ra/coordinator/src/main/java/io/narayana/lra/coordinator/api/CoordinatorContainerFilter.java

+
+        String apiVersionString = requestContext.getHeaderString(LRAConstants.LRA_API_VERSION_HEADER_NAME);
+
+        // request contains multiple API version headers, using first one and printing a warning


Ok. I would like to leave the logging for the investigation purposes if you are not strictly against such approach. Moving the log level to debug and changing the message.

ochaloup · 2021-03-02T12:55:16Z

rts/lra/API.adoc

+      the same version with `-preRelease` part.
+
+Client may demand behaviour based on particular API version
+by providing HTTP header link:./service-base/src/main/java/io/narayana/lra/LRAConstants.java[`Narayana-LRA-API-version`] on the call.


Yes. But we can state it in the document. Or if you think the behaviour should be "less" strict.
Some text suggestion would be great in such case.

ochaloup · 2021-03-02T12:56:40Z

rts/lra/API.adoc

+in Narayana LRA coordinator service.
+This document is written for developer of Narayana LRA services.
+
+The version format is `major.minor`[`-preRelease`].


Sorry, this is my omission. The text about the preRelease should be removed. The required is major.minor.

ochaloup · 2021-03-02T13:03:42Z

rts/lra/API.adoc

+
+The Narayana LRA REST API is expected to support for at least two previous
+`major` versions (ie. support is expected in parallel at least of versions 1.x,
+2.x and 3.x until the 4.0 is released).


I'm sorry for this. I remember we talked about it but I forgot to include it.

ochaloup · 2021-03-02T13:07:46Z

rts/lra/API.adoc

+which demands (via HTTP header `Narayana-LRA-API-version`) version
+higher than the current API version (ie. the highest version that the API
+is created for).
+The unsupported version could be one that is already deprecated


Right, agree. I took your text just the last part seems to me duplicate to the items above. I didn't include it to PR. If that was wrong understanding, let me know I will change the text.

ochaloup · 2021-03-02T13:14:40Z

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

-                        .get()
-                        .get(QUERY_TIMEOUT, TimeUnit.SECONDS);
+                .request()
+                .header(LRA_API_VERSION_HEADER_NAME, LRAConstants.CURRENT_API_VERSION_STRING)


The NarayanaLRAClient is a library which can be used in Java for the call to coordinator. I would suggest to users to use it. But if a user wants to run a service which is not MP compatible or if other language is used then information about API versioning is a good idea.

ochaloup · 2021-03-02T13:17:36Z

rts/lra/service-base/src/main/java/io/narayana/lra/LRAConstants.java

+
+    /**
+     * The Narayana API version for LRA coordinator supported for the release.
+     * Any higher version is considered as unimplemented and unknown.


I added that note to the prior javadoc section.

ochaloup · 2021-03-02T13:17:57Z

rts/lra/service-base/src/main/java/io/narayana/lra/logging/lraI18NLogger.java

            "Reason: '%s'")
    void warn_move_lra_record(String failedUid, String exceptionMessage);

+    @LogMessage(level = WARN)
+    @Message(id = 25029, value = "Multiple version headers '%s' were provided for the request '%s'. " +
+            "Consider to provide only one such header, there will be used value '%s' for now.")


I removed this.

ochaloup · 2021-03-02T14:28:57Z

...tension/src/main/java/io/narayana/lra/arquillian/AppServerCoordinatorDeploymentObserver.java

@@ -53,15 +53,15 @@
     * which is deployed when the app server starts.
     */
    public void handleAfterStartup(@Observes AfterStart event, Container container) throws Exception {
-        if(!container.getName().contains(CONTAINER_NAME_RECOGNITION)) {
+        if (!container.getName().contains(CONTAINER_NAME_RECOGNITION)) {


There was a bit of text in the definition of the field. I've extended it a little bit now.

ochaloup · 2021-03-02T14:31:50Z

...st/arquillian-extension/src/main/java/io/narayana/lra/arquillian/ArquillianParametrized.java

+import java.util.List;
+
+/**
+ * Parameterized Arquillian JUnit runner adding the possibility to run parameterized JUnit test cases ,


The parametrized JUnit tests are tests which runs several times with different parameters - e.g. something here https://www.tutorialspoint.com/junit/junit_parameterized_test.htm
This is an adjustment of the Arquillian runner to be capable to run in the similar manner. It's for a generic use. The point is to use it for versions right now but it can be used in different tests as well.

jbosstm-bot · 2021-03-02T14:35:42Z

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

ochaloup · 2021-03-02T14:35:57Z

@mayankkunwar @jmfinelli per Mike's comment, do you think you would have time to take a look at this PR and review the testcases (CoordinatorApiIT.java, CoordinatorApiWrongVersionIT.java , ValidTestVersions.java, ValidTestVersionsRule.java) and the handling around?

jbosstm-bot · 2021-03-02T15:10:04Z

LRA profile tests failed (http://narayanaci1.eng.hst.ams2.redhat.com/job/btny-pulls-narayana/PROFILE=LRA,jdk=jdk8.latest,label=linux/116/): narayana build failed

jbosstm-bot · 2021-03-02T15:50:47Z

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

jbosstm-bot · 2021-03-02T18:39:53Z

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

jmfinelli

As per @mmusgrov request, I reviewed the testing part of this PR. My main points are about using a tuple instead of an array of String to parameterise the Test Class CoordinatorApiIT.java and using TestRule instead of MethodRule. The rest of the comments are about clarifying error messages and descriptions of test classes. Please, find comments below.

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/ValidTestVersionsRule.java

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/ValidTestVersions.java

jmfinelli · 2021-03-03T18:46:34Z

...st/arquillian-extension/src/main/java/io/narayana/lra/arquillian/ArquillianParametrized.java

+import java.util.List;
+
+/**
+ * Parameterized Arquillian JUnit runner adding the possibility to run parameterized JUnit test cases ,


+1

I like that you introduced parameters in the Arquillian runner. Good idea :-)

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApiIT.java

…or API

ochaloup

Thanks @jmfinelli for your exhaustive review. I tried to address your notes. Please take a look.

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApiIT.java

...ra/test/basic/src/test/java/io/narayana/lra/arquillian/api/CoordinatorApiWrongVersionIT.java

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/ValidTestVersions.java

rts/lra/test/basic/src/test/java/io/narayana/lra/arquillian/api/ValidTestVersionsRule.java

jbosstm-bot · 2021-03-05T16:55:47Z

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

jbosstm-bot · 2021-03-05T18:49:52Z

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

ochaloup · 2021-03-09T17:20:41Z

@jmfinelli thanks for the review and the approval :-)
@mmusgrov is fine by you to proceed with merge?

Let's go with Manuel's one instead.

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 7b355fe to cfedc9a Compare February 10, 2021 17:30

xstefank reviewed Feb 10, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from cfedc9a to 2f0f7db Compare February 11, 2021 12:15

mmusgrov reviewed Feb 11, 2021

View reviewed changes

ochaloup commented Feb 14, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 2f0f7db to 349bcf3 Compare February 14, 2021 08:59

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 349bcf3 to 5b49630 Compare February 14, 2021 14:22

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 5b49630 to c2153a4 Compare February 15, 2021 10:42

mmusgrov requested changes Feb 16, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from c2153a4 to 02d8bd3 Compare February 16, 2021 19:20

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 02d8bd3 to 99b8804 Compare February 16, 2021 19:59

ochaloup commented Feb 19, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 5c5d3d8 to c8fc322 Compare February 19, 2021 14:46

[DEPENDENCY CLEANUP] removing the default dependencies from top-level…

279b385

… LRA pom.xml

ochaloup force-pushed the JBTM-3294-http-version-headers branch from c8fc322 to 4d93f33 Compare March 2, 2021 06:58

mmusgrov previously requested changes Mar 2, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 4d93f33 to 969fec6 Compare March 2, 2021 14:32

ochaloup commented Mar 2, 2021

View reviewed changes

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 969fec6 to 308a03f Compare March 2, 2021 15:47

jmfinelli requested changes Mar 4, 2021

View reviewed changes

[JBTM-3294] adding version HTTP headers version handling to coordinat…

e4875c7

…or API

ochaloup force-pushed the JBTM-3294-http-version-headers branch from 308a03f to e4875c7 Compare March 5, 2021 16:50

ochaloup commented Mar 5, 2021

View reviewed changes

jmfinelli approved these changes Mar 9, 2021

View reviewed changes

ochaloup merged commit 9adcde5 into jbosstm:master Mar 10, 2021


		return Response.status(status).build();
		private void verifyVersion(String versionString) {


		String apiVersionString = requestContext.getHeaderString(LRAConstants.LRA_API_VERSION_HEADER_NAME);

		// request contains multiple API version headers, using first one and printing a warning

[JBTM-3294] adding HTTP version headers for coordinator #1783

[JBTM-3294] adding HTTP version headers for coordinator #1783

Conversation

ochaloup commented Feb 10, 2021

jbosstm-bot commented Feb 10, 2021

jbosstm-bot commented Feb 10, 2021

jbosstm-bot commented Feb 10, 2021

jbosstm-bot commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ochaloup Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbosstm-bot commented Feb 11, 2021

jbosstm-bot commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ochaloup left a comment

Choose a reason for hiding this comment

ochaloup Feb 13, 2021 • edited

Choose a reason for hiding this comment

jbosstm-bot commented Feb 14, 2021

jbosstm-bot commented Feb 14, 2021

ochaloup commented Feb 15, 2021

jbosstm-bot commented Feb 15, 2021

jbosstm-bot commented Feb 15, 2021

mmusgrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusgrov commented Feb 16, 2021

mmusgrov commented Feb 16, 2021

mmusgrov commented Feb 16, 2021

jbosstm-bot commented Feb 16, 2021

jbosstm-bot commented Feb 16, 2021

ochaloup commented Feb 16, 2021

jbosstm-bot commented Feb 16, 2021

jbosstm-bot commented Feb 16, 2021

ochaloup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbosstm-bot commented Feb 19, 2021

jbosstm-bot commented Feb 19, 2021

jbosstm-bot commented Feb 19, 2021

jbosstm-bot commented Feb 19, 2021

jbosstm-bot commented Mar 2, 2021

ochaloup commented Mar 2, 2021

jbosstm-bot commented Mar 2, 2021

mmusgrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ochaloup Feb 11, 2021 •

edited

ochaloup Feb 13, 2021 •

edited