-
Notifications
You must be signed in to change notification settings - Fork 32
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
Initial spec doc contribution for Jakarta EE #275
Initial spec doc contribution for Jakarta EE #275
Conversation
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
….adoc Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
Signed-off-by: Leandro Coutinho <lescoutinhovr@gmail.com>
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.
References to JCA need to change to Jakarta Connectors instead of Jakarta EE Connector Architecture
References to EJB need to change to Enterprise Beans or enterprise beans (most references are to EJB containers)
References to JTA need to change to Jakarta transactions or just transactions
|
||
For historical reasons JMS offers four alternative sets of interfaces | ||
For historical reasons Jakarta Messaging offers three alternative sets of interfaces |
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.
Why change to three - (1.0 = 2, 1.1 = 1, 2.0 = 1 = four)
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 reverted this back to "four"
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
@hussainnm, I'm going to fix the wrong references to JCA, EJB and JTA. I'm not sure whether to change references to "EJB Container" because the spec in the master branch of Jakarta Enterprise Beans currently still defines it as "EJB Container". I also asked about it in the Enterprise Beans github issue here: jakartaee/enterprise-beans#99 (comment) |
The Enterprise Beans PR https://github.com/eclipse-ee4j/ejb-api/pull/96/files#diff-adade127f19060d71cb75f896da4c40cR89 proposes to change "EJB Container" to "Enterprise Beans Container". I'll replace "EJB Container" to match that. |
The scope section was redundant after we added the original JMS spec content. Enterprise Beans container is used instead of EJB container as currently proposed by the Enterprise Beans spec but the final name is not yet approved. Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
JTA transaction replaced with Jakarta transaction Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
@hussainnm, I've replaced all references to JCA, EJB and JTA:
I also removed the scope section which became redundant. It caused that the section numbers were shifted and it was there only as a stub before the Java EE document was donated. |
commit and rollback methods in this context throws a Jakarta Messaging | ||
TransactionInProgressException. | ||
|
||
==== Distributed transactions | ||
|
||
Jakarta Messaging does not require that a provider support distributed transactions; | ||
however, it does define that if a provider supplies this support it | ||
should be done via the JTA XAResource API. | ||
should be done via the Jakarta Transactions XAResource API. |
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.
XAResource API is still under JDK and not part of Jakarta Transactions.
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.
Change yo Java XA Resource API as suggested.
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.
Minor changes:
2 occurrences of javax.transaction.UserTransaction
1 occurrence of javax.inject.Inject
XAResource API is still under JDK, this could be changed to Java XAResource API instead of Jakarta Transactions XAResource API
Co-authored-by: hussainnm <mailathussain@yahoo.com>
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
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 bear with me, I am reviewing it randomly and not going through it line by line. Here are two more issues i found
2 occurrences of Java™ Platform, Enterprise Edition needs to be changed to Jakarta EE Platform.
9 occurrences of missing sections, search for empty quotes.
client. Some providers may supply such a triggering mechanism via their | ||
administrative facilities. | ||
|
||
=== Request/reply | ||
|
||
JMS provides the JMSReplyTo message header field for specifying the | ||
Jakarta Messaging provides the JMSReplyTo message header field for specifying the | ||
Destination where a reply to a message should be sent. The | ||
JMSCorrelationID header field of the reply can be used to reference the | ||
original request. See Section “” for more information. |
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 total 9 missing references like this. You can search for empty quotes in the document.
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 were more, I found 16 missing references. I copied the references from the JMS 2.0 specification. I also checked that the section numbers are correct and they are.
|
||
The JMS API is designed to be suitable for use by both Java client | ||
The Jakarta Messaging API is designed to be suitable for use by both Java client | ||
applications using the Java™ Platform, Standard Edition (Java SE), and | ||
Java middle-tier services using the Java™ Platform, Enterprise Edition |
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.
2 occurrences of Java™ Platform, Enterprise Edition needs to be changed to Jakarta EE Platform.
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've fixed those references.
I also removed references to specific versions of Jakarta EE platform or Enterprise Beans as they obsolete with every new version. They were confusing anyway because they referred to an older version of Java EE because the JMS spec was finalized before the version of Java EE spec that included it. If we don't use the exact version, it should be implicitly the Jakarta EE version that includes this version of Jakarta Messaging.
No problem with raising more issues as you continue reviewing the document, @hussainnm. I'm glad that you're so thorough. Is it OK if I keep fixing issues you report as soon as possible or if I wait until you're finished with your review? What's more convenient for you? |
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
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 another missing underscore in 3.8.1.1. Message selector syntax on L1442
Table 8-1 and 8-2 do not need header in section 8.5. Receiving messages synchronously and
8.6. Receiving message bodies synchronously respectively
Add Code blocks in 7.10 JMSProducer method chaining, 12.4.3 Injection Syntax, Chapter 14 and 15
Also review and indent the code within code blocks. There are a few unnecessary backslashes().
Chapter 16 was Appendix A in the original document. There are three references A.1, A1.8, A1.9. You have two options:
- change Chapter 16 to Appendix A or
- change the references to Chapter 16, 16.1.8, 16.1.9 respectively.
Just a general observation, the keywords like class names, method names, etc., need to be in monospace (``) for better readability. You could do this in a separate PR.
@@ -1245,18 +1241,18 @@ properties are undefined. | |||
|
|||
==== Provider-specific properties | |||
|
|||
JMS reserves the ‘JMS___<vendor_name>__’ property name prefix for | |||
Jakarta Messaging reserves the ‘JMS___<vendor_name>__’ property name prefix for |
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.
Fixed.
@@ -3102,7 +3101,7 @@ send(Destination destination, Message message) | |||
|
|||
____ |
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.
Fixed.
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
And a formatting fix Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
Thanks for more comments, @hussainnm. I believe I've fixed all except I didn't change keywords in the text as it would be too much work now and also would pollute the diff for this PR. I may raise a separate PR after this is merged. Here's what I've fixed today:
|
try (JMSContext context=connectionFactory.createContext();){ | ||
context.createProducer().send(dataQueue,body); | ||
} | ||
} | ||
|
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.
Missing closing block (----), this is messing rest of the document.
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 afraid this could happen, I should've doublechecked. This is fixed and I also fixed it in one more place which Asciidoctor reported as missing ending element.
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
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.
Line 6052 and 6074 is missing closing block.
Signed-off-by: Ondro Mihalyi <ondrej.mihalyi@payara.fish>
Fixed. There were a few more which I discovered using a regex search. |
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.
LGTM
Amazing work everyone! Thanks a bunch :) I'll merge this now. If there are additional changes needed they can be done in follow-up PRs. |
Initial modifications by @tivrfoa of the donated document for Jakarta EE. This is a follow-up for #267, which was closed before merged.
This pull request builds on #273. After #273, this PR will only contain changes made in the donated document to make it easier to review.
(This PR replaces #274 which wasn't synchronized with master and didn't show only diffs in the spec file)