You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/07/15 16:39:15 UTC
[GitHub] [activemq] mattrpav opened a new pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
mattrpav opened a new pull request #682:
URL: https://github.com/apache/activemq/pull/682
- API update only
- Throw UnsupportedOperationException
- Disable activemq-camel from build
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888484005
>
> Am I getting this right:
>
> 1. App currently uses JMS 2.0 API
>
> 2. App DeliveryDelay is required, and sets DeliveryDelay on all messages through the JMS 2.0 MessageProducer
>
> 3. App has a way to handle that ActiveMQ (or other JMS 1.1) doesn't have this method so it doesn't fail
>
> 4. Upon upgrade of activemq-client jar, this same unchanged code will now throw an exception
>
No. App can use JMS 1.1 or JMS 2.0 API (EDIT: actually yes, the issue will only arise if they use 2.0 to be fair...which would be more likely given its the aim of this change). Configuring DeliveryDelay at all is not required. App sends JMS 1.1 providers message object, through JMS 2.0 client (which tolerates 1.1 messages). After this change as-is, that will fail all the time during send.
Its possible for a JMS 2.0 client to work upon and send 1.1 message objects, tolerating that the message.setDeliveryTime() method is defined but is not being implemented by the older object (as clearly it didnt exist before, and so couldnt be implemented, but backwards compatibility is the name of the game for JMS). The JMS reference impl definitely did this.
Its possible for a JMS 2.0 client to tolerate being used only with the 1.1 API jar and for the message.setDeliveryTime() method to not actually be defined at all at runtime. I dont recall if the RI did this.
Qpid JMS for example does both of these things. It will let you send a 1.1 message through it, and it will operate using the 1.1 API jar. It simply made sense to support when transitioning specs, not dropping support for using it with existing 1.1 providers (e.g 5.x) or existing applications, when they couldn't actually be doing anything that wasnt still supported as all that API all remained precisely for compatibility. Those are very different situations from the method being defined, implemented and explicitly throwing UOE, and I dont expect anything to really handle that it really makes no sense for it to ever do so, especially for a long setter that is simply for recording 'what actually happened during send'.
Note that all sends will fail, regardless whether a delivery delay is configured on the producer, as a value is always being set on the sent messages to either indicate the actual delivery time or ensure it records there was no delay (as the sent message object may previously have had a deliveryTime value, so that needs cleared to ensure the getter reflects reality).
>
> I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem.
>
> As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
>
There is no delay unless the feature is implemented (which would remove any need for a discussion), so there cant really be a sillent wiping. The message setters dont control the delay as already discussed, and why would anyone expect the message to be reporting a non-zero delivery time after send, when producer.setDeliveryDelay(..) is throwing to indicate a delay cant actually be configured?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-889138211
> > Cool, what is the link to the test code? Test output?
>
> The test code is at https://github.com/apache/qpid-jms/blob/1.1.0/qpid-jms-interop-tests/qpid-jms-activemq-tests/src/test/java/org/apache/qpid/jms/producer/JmsMessageProducerTest.java#L177. Its very simple, 'create foreign 5.x message object, try to send + receive message'. It fails to send on line 200, due to JMSDeliveryTime UOE from the message.
>
> For completeness, this is the change I made to try out the changes: [gemmellr/qpid-jms@516462f](https://github.com/gemmellr/qpid-jms/commit/516462f9d177ab8d1ab48fe29555e464c172201f)
Without a stacktrace, I'm estimating that this is the method throwing the exception:
```
JMSSession.java
private void setForeignMessageDeliveryTime(Message foreignMessage, long deliveryTime) throws JMSException
```
Yeah, so in that code it could be argued that the 'hasDelay' marker variable should be used before calling the setJMSDeliveryTime in the private setForeignMessageDeliveryTime method. That method presumes that it should call the setJMSDeliveryTime even if there hasn't been a DeliveryDelay specified.
Either approach (throw UOE or swallow expected Delivery Delays) will present downside to varying users and interests.
I contend that the current UOE approach matches several project goals:
1. The implementation matches the description in the JIRA
2. The mailing list discussion has settled on this approach
3. The desire to keep PR changes concise
There is a dedicated JIRA for DeliveryDelay implentation discussion here: [AMQ-8320](https://issues.apache.org/jira/browse/AMQ-8320)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-893174170
@rmannibucau yes, it's what I have in mind as well, as a first step (and what I started). @mattrpav took more "ambitious" approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892952534
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of `JMS20CompatibilityTest`s, that utilize the APIs and act accordingly.
The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
How does that sound? The action items there would be:
- @mattrpav to add a series of basic tests that cover the altered APIs
- @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that _does not_ throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888910656
> Cool, what is the link to the test code? Test output?
The test code is at https://github.com/apache/qpid-jms/blob/1.1.0/qpid-jms-interop-tests/qpid-jms-activemq-tests/src/test/java/org/apache/qpid/jms/producer/JmsMessageProducerTest.java#L177. Its very simple, 'create foreign 5.x message object, try to send + receive message'. It fails to send on line 200, due to JMSDeliveryTime UOE from the message.
For completeness, this is the change I made to try out the changes: https://github.com/gemmellr/qpid-jms/commit/516462f9d177ab8d1ab48fe29555e464c172201f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav merged pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav merged pull request #682:
URL: https://github.com/apache/activemq/pull/682
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737003423
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
i see that this PR is still using jakartainstead of geronimo , was the mentioned osgi compatibility, whats the story/consensus, from above it seems that it consensus was to use geronimo but PR isnt updated from that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737004250
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -66,7 +91,7 @@ public void setDisableMessageID(boolean value) throws JMSException {
this.disableMessageID = value;
}
- /**
+ /**
Review comment:
nit: indentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r742267823
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
##########
@@ -162,6 +162,44 @@ public TopicSession createTopicSession(boolean transacted, int ackMode) throws J
return (TopicSession) createSession(transacted, ackMode);
}
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
+ public Session createSession() throws JMSException {
+ throw new UnsupportedOperationException("createSession() is unsupported");
+ }
+
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @param acknowledgeMode indicates whether the consumer or the client will
+ * acknowledge any messages it receives; ignored if the
+ * session is transacted. Legal values are
+ * <code>Session.AUTO_ACKNOWLEDGE</code>,
+ * <code>Session.CLIENT_ACKNOWLEDGE</code>, and
+ * <code>Session.DUPS_OK_ACKNOWLEDGE</code>.
+ * @return a newly created session
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @see Session#AUTO_ACKNOWLEDGE
+ * @see Session#CLIENT_ACKNOWLEDGE
+ * @see Session#DUPS_OK_ACKNOWLEDGE
+ * @since 2.0
+ */
+ @Override
+ public Session createSession(int sessionMode) throws JMSException {
+ throw new UnsupportedOperationException("createSession(int sessionMode) is unsupported");
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(int sessionMode) {
+ throw new UnsupportedOperationException("createContext(sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] rmannibucau commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r671524041
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
Osgi support, will likely not hit jakarta anytime soon whereas G does/will maintain it so best choice stays G as of today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r742267823
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
##########
@@ -162,6 +162,44 @@ public TopicSession createTopicSession(boolean transacted, int ackMode) throws J
return (TopicSession) createSession(transacted, ackMode);
}
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
+ public Session createSession() throws JMSException {
+ throw new UnsupportedOperationException("createSession() is unsupported");
+ }
+
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @param acknowledgeMode indicates whether the consumer or the client will
+ * acknowledge any messages it receives; ignored if the
+ * session is transacted. Legal values are
+ * <code>Session.AUTO_ACKNOWLEDGE</code>,
+ * <code>Session.CLIENT_ACKNOWLEDGE</code>, and
+ * <code>Session.DUPS_OK_ACKNOWLEDGE</code>.
+ * @return a newly created session
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @see Session#AUTO_ACKNOWLEDGE
+ * @see Session#CLIENT_ACKNOWLEDGE
+ * @see Session#DUPS_OK_ACKNOWLEDGE
+ * @since 2.0
+ */
+ @Override
+ public Session createSession(int sessionMode) throws JMSException {
+ throw new UnsupportedOperationException("createSession(int sessionMode) is unsupported");
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(int sessionMode) {
+ throw new UnsupportedOperationException("createContext(sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -66,7 +91,7 @@ public void setDisableMessageID(boolean value) throws JMSException {
this.disableMessageID = value;
}
- /**
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
+ }
+
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
fixed
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
##########
@@ -162,6 +162,44 @@ public TopicSession createTopicSession(boolean transacted, int ackMode) throws J
return (TopicSession) createSession(transacted, ackMode);
}
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
+ public Session createSession() throws JMSException {
+ throw new UnsupportedOperationException("createSession() is unsupported");
+ }
+
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @param acknowledgeMode indicates whether the consumer or the client will
+ * acknowledge any messages it receives; ignored if the
+ * session is transacted. Legal values are
+ * <code>Session.AUTO_ACKNOWLEDGE</code>,
+ * <code>Session.CLIENT_ACKNOWLEDGE</code>, and
+ * <code>Session.DUPS_OK_ACKNOWLEDGE</code>.
+ * @return a newly created session
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @see Session#AUTO_ACKNOWLEDGE
+ * @see Session#CLIENT_ACKNOWLEDGE
+ * @see Session#DUPS_OK_ACKNOWLEDGE
+ * @since 2.0
+ */
+ @Override
+ public Session createSession(int sessionMode) throws JMSException {
+ throw new UnsupportedOperationException("createSession(int sessionMode) is unsupported");
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(int sessionMode) {
+ throw new UnsupportedOperationException("createContext(sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -66,7 +91,7 @@ public void setDisableMessageID(boolean value) throws JMSException {
this.disableMessageID = value;
}
- /**
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
+ }
+
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-880850655
As discussed on the mailing list, I did it already and I was about to create the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888154727
> That is an extreme edge use case-- one that is acceptable for a RC release.
I'm not sure I'd agree that bridging between different systems was 'extreme-edge'. Not the primary use, sure, but hardly strange and unusual either.
Not sure I have seen mention of doing 'RC releases' before. Are these to be named 5.17.0-rc1, rc2 etc given it is targeting main, or is it for after that as a series of 5.18.0-rcX? Might seem odd to use 5.17.0 given that should probably be out and done already (and would then be looking for 5.17.1 etc for bug fixes), while this is unimplemented. Given that it even seems a stretch calling it an -rc, to me that usually implies near-completion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-887664563
> Note that by specifically adding an implementation of setJMSDeliveryTime that throws UOE, this change will likely actually break sending of ActiveMQ 5.x messages through other existing JMS 2 providers that handle being given JMS 1.x message objects not implementing the method, as they wont handle it being implemented but deliberately throwing UOE. Unlike the session etc they cant check any version metadata first to avoid the method as all they see is the message.
@gemmellr That is an extreme edge use case-- one that is acceptable for a RC release. These JMS 2.0 methods won't be unimplemented for long. Feel free to add a documentation note if you know of some users that have this scenario and would be impacted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888350778
> Based on how slowly things can happen sometimes I would push back against saying anything like "these methods won't be unimplemented for long" as who knows if/when it will be finished.
@cshannon that's fair. Check out the approach notes on the impl approach on the DeliveryDelay specific sub-task: https://issues.apache.org/jira/browse/AMQ-8320
Basically, I think we can do a first-pass without OpenWire changes, and then tee-up a OpenWire v13 ticket to group up enhancements and minimize the protocol impact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelandrepearce commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892962330
It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did.
I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
Sent from my iPad
> On 4 Aug 2021, at 21:29, ehossack-aws ***@***.***> wrote:
>
>
> I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of JMS20CompatibilityTests, that utilize the APIs and act accordingly.
> The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
> A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
>
> As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
>
> That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
> How does that sound? The action items there would be:
>
> @mattrpav to add a series of basic tests that cover the altered APIs
> @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
> (admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that does not throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.
> Triage notifications on the go with GitHub Mobile for iOS or Android.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] tabish121 commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
tabish121 commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892970530
> It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did. I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
For clarity this has nothing to do with broker side AMQP support, the issue seen from Qpid bridging tests simply exposes an issue with the proposed Openwire client changes which don't represent a complete implementation of the JMS 2.0 specification. It's quite likely that other libraries such as PooledJMS would similarly encounter unexpected situations as it also uses a similar mechanism as the Qpid client does to detect JMS 1.1 clients operating with JMS 2.0 API jars on the classpath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892982004
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737002065
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
##########
@@ -162,6 +162,44 @@ public TopicSession createTopicSession(boolean transacted, int ackMode) throws J
return (TopicSession) createSession(transacted, ackMode);
}
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
+ public Session createSession() throws JMSException {
+ throw new UnsupportedOperationException("createSession() is unsupported");
+ }
+
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @param acknowledgeMode indicates whether the consumer or the client will
+ * acknowledge any messages it receives; ignored if the
+ * session is transacted. Legal values are
+ * <code>Session.AUTO_ACKNOWLEDGE</code>,
+ * <code>Session.CLIENT_ACKNOWLEDGE</code>, and
+ * <code>Session.DUPS_OK_ACKNOWLEDGE</code>.
+ * @return a newly created session
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @see Session#AUTO_ACKNOWLEDGE
+ * @see Session#CLIENT_ACKNOWLEDGE
+ * @see Session#DUPS_OK_ACKNOWLEDGE
+ * @since 2.0
+ */
+ @Override
+ public Session createSession(int sessionMode) throws JMSException {
+ throw new UnsupportedOperationException("createSession(int sessionMode) is unsupported");
Review comment:
nit: indentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-952966248
> seems some code style clean up needs to be done in general before merge, indentation a little over the place in many files.
I'm on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-881566639
>Either way, the JEE transition is going to force framework devs and users into being savvy with Maven and I do not think there is any way around that.
>
> I'd be interested in hearing others' perspective as well.
@mattrpav for a usability perspective, whatever approach we take, I think just being helpful on the website is sufficient -> have a page that clearly states what is supported for JMS, what isn't, what would be the required/removed dependencies for maven, gradle and some concise examples
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892982004
What exactly is breaking? If we put a long in there, it’s a dead store.
The pre-openwire v13 change is to map to a property for the interim. See the ticket specifically about DeliveryTime.
My suggestion is to move forward with this set of changes and then I can push the PR w the property-based DeliveryTime solution and this concern goes away as DeliveryTime will be implemented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r673973590
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
I have some more changes coming in this morning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888484005
>
> Am I getting this right:
>
> 1. App currently uses JMS 2.0 API
>
> 2. App DeliveryDelay is required, and sets DeliveryDelay on all messages through the JMS 2.0 MessageProducer
>
> 3. App has a way to handle that ActiveMQ (or other JMS 1.1) doesn't have this method so it doesn't fail
>
> 4. Upon upgrade of activemq-client jar, this same unchanged code will now throw an exception
>
No. App can use JMS 1.1 or JMS 2.0 API (EDIT: actually yes, the issue will only arise if they use 2.0 to be fair...which would be more likely given its the aim of this change). Configuring DeliveryDelay at all is not required. App sends JMS 1.1 providers message object, through JMS 2.0 client (which tolerates 1.1 messages). After this change as-is, making the '1.1 provider' use the 2.0 API and throw UOE for impl, that will fail all the time during send.
Its possible for a JMS 2.0 client to work upon and send 1.1 message objects, tolerating that the message.setDeliveryTime() method is defined but is not being implemented by the older object (as clearly it didnt exist before, and so couldnt be implemented, but backwards compatibility is the name of the game for JMS). The JMS reference impl definitely did this.
Its possible for a JMS 2.0 client to tolerate being used only with the 1.1 API jar and for the message.setDeliveryTime() method to not actually be defined at all at runtime. I dont recall if the RI did this.
Qpid JMS for example does both of these things. It will let you send a 1.1 message through it, and it will operate using the 1.1 API jar. It simply made sense to support when transitioning specs, not dropping support for using it with existing 1.1 providers (e.g 5.x) or existing applications, when they couldn't actually be doing anything that wasnt still supported as all that API all remained precisely for compatibility. Those are very different situations from the method being defined, implemented and explicitly throwing UOE, and I dont expect anything to really handle that it really makes no sense for it to ever do so, especially for a long setter that is simply for recording 'what actually happened during send'.
Note that all sends will fail, regardless whether a delivery delay is configured on the producer, as a value is always being set on the sent messages to either indicate the actual delivery time or ensure it records there was no delay (as the sent message object may previously have had a deliveryTime value, so that needs cleared to ensure the getter reflects reality).
>
> I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem.
>
> As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
>
There is no delay unless the feature is implemented (which would remove any need for a discussion), so there cant really be a sillent wiping. The message setters dont control the delay as already discussed, and why would anyone expect the message to be reporting a non-zero delivery time after send, when producer.setDeliveryDelay(..) is throwing to indicate a delay cant actually be configured?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-887672390
> > Note that by specifically adding an implementation of setJMSDeliveryTime that throws UOE, this change will likely actually break sending of ActiveMQ 5.x messages through other existing JMS 2 providers that handle being given JMS 1.x message objects not implementing the method, as they wont handle it being implemented but deliberately throwing UOE. Unlike the session etc they cant check any version metadata first to avoid the method as all they see is the message.
>
> @gemmellr That is an extreme edge use case-- one that is acceptable for a RC release. These JMS 2.0 methods won't be unimplemented for long. Feel free to add a documentation note if you know of some users that have this scenario and would be impacted.
Based on how slowly things can happen sometimes I would push back against saying anything like "these methods won't be unimplemented for long" as who knows if/when it will be finished.
@gemmellr - Can you describe this scenario in more detail or a specific use case? I want to make sure we are not breaking anything with throwing unimplemented exceptions and how big of an edge case it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r671797478
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
That's my point, agree with Romain: it's my proposal, don't use Jakarta but rather Geronimo Spec instead. It's what other projects are doing (especially Camel). Geronimo Spec supports OSGi cleanly, so, it's better to use it instead of Jakarta.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-928049505
@mattrpav can you please squash all commits in one ? If you don't know how to do it, just ping me on slack or I can do it for you. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-949441040
I'm quicly fixing https://issues.apache.org/jira/browse/AMQ-8408 to perform a new test series on this PR. I will merge when all tests are OK (including OSGi/Karaf ones).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884898168
"Either way, the JEE transition is going to force framework devs and users into being savvy with Maven and I do not think there is any way around that."
Its really not that dissimilar to the situation where where people could use any of the many particular 2.0 API jars they desire in their applications already, even with a 1.1 impl if they want to, prior to this change (which I dont think its a good idea personally).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r742267823
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
##########
@@ -162,6 +162,44 @@ public TopicSession createTopicSession(boolean transacted, int ackMode) throws J
return (TopicSession) createSession(transacted, ackMode);
}
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
+ public Session createSession() throws JMSException {
+ throw new UnsupportedOperationException("createSession() is unsupported");
+ }
+
+ /**
+ * Creates a <CODE>Session</CODE> object.
+ *
+ * @param acknowledgeMode indicates whether the consumer or the client will
+ * acknowledge any messages it receives; ignored if the
+ * session is transacted. Legal values are
+ * <code>Session.AUTO_ACKNOWLEDGE</code>,
+ * <code>Session.CLIENT_ACKNOWLEDGE</code>, and
+ * <code>Session.DUPS_OK_ACKNOWLEDGE</code>.
+ * @return a newly created session
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @see Session#AUTO_ACKNOWLEDGE
+ * @see Session#CLIENT_ACKNOWLEDGE
+ * @see Session#DUPS_OK_ACKNOWLEDGE
+ * @since 2.0
+ */
+ @Override
+ public Session createSession(int sessionMode) throws JMSException {
+ throw new UnsupportedOperationException("createSession(int sessionMode) is unsupported");
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(int sessionMode) {
+ throw new UnsupportedOperationException("createContext(sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -66,7 +91,7 @@ public void setDisableMessageID(boolean value) throws JMSException {
this.disableMessageID = value;
}
- /**
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
+ }
+
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] rmannibucau commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-893172819
Maybe stupid but in tomee to have jms2 api support i wrapped amq5 in "TomEEAMQ" classes.
This means a first compatibility version can be done in a jms2 package enabling a full jms1 compat in existing package and to start a jms2 support in a new package reusing/delegating to jms1 code.
This is pretty straight forward and fast to do without any regression.
Only change in current code can be in factories with a version config to switch between impl but at the end it is mainly delegation code and works well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] rmannibucau commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884367414
1. We can make it stable now if it helps
2. A EE home at apache as jakarta.x is at eclipse for artifact except it is under a single umbrella vs per spec and spec lead + it supports osgi whereas jakarta does not want to (we asked aiming at dropping these geronimo artifacts)
3. It is the same with jakarta anyway + point 1 so guess it is still worse to use jakarta for users
4. Ultilately it should go at geronimo since it is where the most EE+OSGi work is done lately (including capabilities)
So overall if you want OSGi you have to use geronimo, if not it is asf vs eclipse only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-887666101
> 1. We can make it stable now if it helps
>
> 2. A EE home at apache as jakarta.x is at eclipse for artifact except it is under a single umbrella vs per spec and spec lead + it supports osgi whereas jakarta does not want to (we asked aiming at dropping these geronimo artifacts)
>
> 3. It is the same with jakarta anyway + point 1 so guess it is still worse to use jakarta for users
>
> 4. Ultilately it should go at geronimo since it is where the most EE+OSGi work is done lately (including capabilities)
>
>
> So overall if you want OSGi you have to use geronimo, if not it is asf vs eclipse only.
@rmannibucau I know what Geronimo is ;-). My point was to consider the perspective of new-to-Java and new-to-JMS users that are using ActiveMQ for the first time.
The latest version of the PR provides gerinomo for OSGi/Karaf users (myself included) and has a the Jakarta jar for non-OSGi use cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888163344
> @gemmellr - Can you describe this scenario in more detail or a specific use case? I want to make sure we are not breaking anything with throwing unimplemented exceptions and how big of an edge case it is.
Take an ActiveMQ 5 JMS message object, and send that JMS Message somewhere else using a different JMS client. E.g receive from a 5.x broker and publish using the same message object to another type of broker...could be due to communicating with different teams, companies, or handling a system transition, etc.
As part of that process if the other provider is actually a JMS 2 client, it will try to stamp the JMSDeliveryTime on the original message object like its meant to. It may have handled the method not being implemented at all on JMS 1.x message objects since thats not so unexpected due to spec transition, but will likely fail if the method actually exists but simply throws UOE as thats unexpected.
Basically im saying implement the setter method and at least make 5.x call it to set it to 0 during send.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884328074
Up vote for geronimo-jms_2.0_spec-1.0-alpha-2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-881557987
@jbonofre this change is purely packaging skeleton. I know you were working on full JMS 2.0 support along w/ big things like replicated-kahadb. I thought I'd pitch in to help get the ball rolling on the low-hanging fruit. There is no real code change here, so I think its good to merge and should slide in with your real JMS 2.0 impl work.
Reviewing other projects and frameworks, I think the geronimo vs jakarta is the real question here.
If we go geronimo users will have this to look forward to:
gerinomo 1.1 -> geronimo 2.0 -> jakarta 2.0 -> jakarta 3.0
Alternatively, if we go jakarta now, users have this:
gerinomo 1.1 -> jakarta 2.0 -> jakarta 3.0
CXF has transitioned many spec jars to jakarta, and I suspect Camel will do the same as well. I think the alignment makes more sense for CXF and Camel to consume the dep ActiveMQ (being as ActiveMQ is the JMS provider and those ride on top of), or simply provide the <exclude/>.
Either way, the JEE transition is going to force users into being savvy with Maven <exclude/> and I do not think there is any way around that.
I'd be interested in hearing others' perspective as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-960248600
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-952965783
> Like wise, why as part of JMS change is zookeeper being removed?
Zookeeper was already removed from the code base via a different PR. The only change included here is housekeeping the zookeeper entry from the Karaf features file. Found it while adding the geronimo-jms 2.0 dep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737002257
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(int sessionMode) {
+ throw new UnsupportedOperationException("createContext(sessionMode) is not supported");
+ }
Review comment:
nit: indentation
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
/**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password, int sessionMode) {
+ throw new UnsupportedOperationException("createContext(userName, password, sessionMode) is not supported");
+ }
Review comment:
nit: indentation
##########
File path: activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
##########
@@ -77,8 +78,40 @@ public Connection createConnection(String userName, String password) throws JMSE
i.setPassword(password);
return createConnection(i);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext(userName, password) is not supported");
+ }
Review comment:
nit: indentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888484005
>
> Am I getting this right:
>
> 1. App currently uses JMS 2.0 API
>
> 2. App DeliveryDelay is required, and sets DeliveryDelay on all messages through the JMS 2.0 MessageProducer
>
> 3. App has a way to handle that ActiveMQ (or other JMS 1.1) doesn't have this method so it doesn't fail
>
> 4. Upon upgrade of activemq-client jar, this same unchanged code will now throw an exception
>
No. App can use JMS 1.1 or JMS 2.0 API. Configuring DeliveryDelay at all is not required. App sends JMS 1.1 providers message object, through JMS 2.0 client (which tolerates 1.1 messages). After this change as-is, that will fail all the time during send.
Its possible for a JMS 2.0 client to work upon and send 1.1 message objects, tolerating that the message.setDeliveryTime() method is defined but is not being implemented by the older object (as clearly it didnt exist before, and so couldnt be implemented, but backwards compatibility is the name of the game for JMS). The JMS reference impl definitely did this.
Its possible for a JMS 2.0 client to tolerate being used only with the 1.1 API jar and for the message.setDeliveryTime() method to not actually be defined at all at runtime. I dont recall if the RI did this.
Qpid JMS for example does both of these things. It will let you send a 1.1 message through it, and it will operate using the 1.1 API jar. It simply made sense to support when transitioning specs, not dropping support for using it with existing 1.1 providers (e.g 5.x) or existing applications, when they couldn't actually be doing anything that wasnt still supported as all that API all remained precisely for compatibility. Those are very different situations from the method being defined, implemented and explicitly throwing UOE, and I dont expect anything to really handle that it really makes no sense for it to ever do so, especially for a long setter that is simply for recording 'what actually happened during send'.
Note that all sends will fail, regardless whether a delivery delay is configured on the producer, as a value is always being set on the sent messages to either indicate the actual delivery time or ensure it records there was no delay (as the sent message object may previously have had a deliveryTime value, so that needs cleared to ensure the getter reflects reality).
>
> I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem.
>
> As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
>
There is no delay unless the feature is implemented (which would remove any need for a discussion), so there cant really be a sillent wiping. The message setters dont control the delay as already discussed, and why would anyone expect the message to be reporting a non-zero delivery time after send, when producer.setDeliveryDelay(..) is throwing to indicate a delay cant actually be configured?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884898458
Note that by specifically adding an implementation of setJMSDeliveryTime that throws UOE, this change will likely actually break sending of ActiveMQ 5.x messages through other existing JMS 2 providers that handle being given JMS 1.x message objects not implementing the method, as they wont handle it being implemented but deliberately throwing UOE. Unlike the session etc they cant check any version metadata first to avoid the method as all they see is the message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892985778
@gemmellr all good. I’ll either remove the UPE or push the impl.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-960248600
@mattrpav thanks for sorting the formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-904634515
> Removing the whole of activemq-camel reall seems like something deserving of its own JIRA vs sneaking it into a commit with the title "JMS 2.0 API clean-ups"
@tabish121 activemq-camel removal PR is here: https://github.com/apache/activemq/pull/701
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-952419469
seems some code style clean up needs to be done in general before merge, indentation a little over the place in many files. Like wise, why as part of JMS change is zookeeper being removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737004363
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
Review comment:
nit: indentation
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
+ }
+
+ /**
Review comment:
nit: indentation
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
Review comment:
nit: indentation
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
Review comment:
nit: indentation
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
nit: indentation
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
Review comment:
nit: indentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892982659
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888394147
@mattrpav I'm not actually all that new to JMS at this stage really, I'm at least a little aware what the methods are for and how the functionality is used, having implemented JMS [2] in full including a full protocol mapping of the functionality in a client capable of doing such scheduled delivery against ActiveMQ 5 for several years now already. I have no idea why you think I was talking about application or 'wrapper' usage of the setter, I wasn't.
I think I was pretty clear about taking a 5.x message object and sending it through another JMS 2 client, which is where the issue arises, as it necessarily tries to call setJMSDeliveryTime on every message sent (as either there is a delay, and the actual time needs set, or there isnt and it should be clearing the potentially-non-zero existing value to indicate so). Such a client client may have reasonably handled a JMS 1.1 object not having the method implemented, as e.g. 5.x didnt so far, but it is not going to expect the method to be actually implemented but throw UOE.
These methods exist precisely for this use case, for [foreign] providers to set the needed values on message objects being sent [which arent their own implementation].
> This method is for use by JMS providers only to set this field when a message is sent. This message cannot be used by clients to configure the delivery time of the message. This method is public to allow a JMS provider to set this field when sending a message whose implementation is not its own.
If 5.x is going to use the JMS 2 API then any JMS 2 message object sent with it should reasonably be expected to come back with a delivery time value of 0 after send if there is no delay occurring, i.e it should be doing the equivalent of setJMSDeliveryTime(0) by itself on its own messages, and actually calling setJMSDeliveryTime(0) on other providers messages to clear any existing non-zero value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r673972220
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnectionFactory.java
##########
@@ -271,7 +272,39 @@ public synchronized Connection createConnection(String userName, String password
return newPooledConnection(connection);
}
- protected Connection newPooledConnection(ConnectionPool connection) {
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
Just missed this one.. I'll fix in the follow-up commit this morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-889221247
> Yeah, so in that code it could be argued that the 'hasDelay' marker variable (used 7 lines earlier in L907) should be used before calling the setJMSDeliveryTime in the private setForeignMessageDeliveryTime method. That method presumes that it should call the setJMSDeliveryTime even if there hasn't been a DeliveryDelay specified.
>
It is set for every message sent because it should be. The [foreign] message object may already have a populated JMSDeliveryTime value so even if there is no delay being applied during this send call, the JMSDeliveryTime value should still be stamped appropriately on the sent message object to ensure it is correctly reflecting this send. (The 'hasDelay' variable is used for something quite different, but obviously related).
> Either approach (throw UOE or swallow expected Delivery Delays) will present downside to varying users and interests.
>
The message.set/getJMSDeliveryTime would just work as they are meant to, allowing their use to simply report the values, and not breaking usage of 5.x message object with other providers, its hardly 'swallowing expected delays'.
The message setter and getter can be trivially handled in 5.x (getting and setting a basic long field), without the producer.setDeliveryDelay() method or delivery delay feature being implemented in 5.x, i.e. the producer.setDeliveryDelay() would still throw until actually implemented.
> I contend that the current UOE approach matches several project goals:
>
> 1. The implementation matches the description in the JIRA
>
> 2. The mailing list discussion has settled on this approach
>
I would say that the mailing list discussion has not addressed this particular aspect, and it highly likely noone thought of it until I pointed it out. As a breaking change I think it runs rather counter to the entire idea of using the newer api jar being 'more compatible' as some seem to think.
> 3. The desire to keep PR changes concise
>
Fixing the message method issue would use less characters than throwing the exceptions would so arguably it would be more concise.
Weve spent many many multiples of the time it would take to fix this, just discussing. I'm done, break what you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r674091556
##########
File path: pom.xml
##########
@@ -310,11 +310,13 @@
<artifactId>activemq-all</artifactId>
<version>${project.version}</version>
</dependency>
+ <!-- Remove activemq-camel
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>activemq-camel</artifactId>
<version>${project.version}</version>
</dependency>
+ -->
Review comment:
Agreed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892982659
>
> * @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
>
I have described more than once, and shown a literal code example of what to do for a test of this issue, which is utterly trivial. Ssend the JMS Message objects from 5.x through a foriegn JMS 2 provider impl. Any that implements what the JMS 2 spec says and as a result tries to apply the mandated value to the original message (using the vendor-neutral Message API in place specifically to let it do so) will fail.
>
> (admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that _does not_ throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning,
Because those Message methods are not really a core part of the implementation of Delivery Delay in 5.x itself, which is really part of the producer impl. They have nothing to do with the behaviour applied by 5.x during send, as was noted during this discussion. They are specifically there to ensure foreign providers can record the values _they_ are applying with _their_ support. Anything should be able to call the 5.x message setJMSDeliveryTime and getJMSDeliveryTime methods and expect it to work, regardless whether the 5.x producer actually supports configuring a delivery delay.
> and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
No, thats what will happens before this PR, when the client takes steps to let a JMS 1.1 message object be sent, because obviously it doesnt find the relevant method implemented, it didnt exist. This PR adds the JMS 2.0 method impl (which then throws), meaning the client actually finds it implemented and will proceed to the lower section and actually calling it. Expecting it to work and not throw UOE, because theres really no reason a simple set/get of a mandatory JMS header long value should ever throw. The Qpid JMS client will not be catching UOE's to satisfy such behaviour from ActiveMQ, that in itself would be incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888163344
> @gemmellr - Can you describe this scenario in more detail or a specific use case? I want to make sure we are not breaking anything with throwing unimplemented exceptions and how big of an edge case it is.
Take an ActiveMQ 5 JMS message object, and send that JMS Message somewhere else using a different JMS client. E.g receive from a 5.x broker and publish using the same message object to another type of broker...could be due to communicating with different teams, companies, or handling a system transition, etc.
As part of that process if the other provider is actually a JMS 2 client, it will try to stamp the JMSDeliveryTime on the original message object like its meant to. It may have handled the method not being implemented at all on JMS 1.x message objects since thats not so unexpected due to spec transition, but will likely fail if the method actually exists but simply throws UOE as thats unexpected.
Basically im saying at least implement the setter method and make 5.x call it to set it to 0 during send (also handling the reverse case of sending other messages through 5.x, ensuring they are properly stamped).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r671368634
##########
File path: pom.xml
##########
@@ -310,11 +310,13 @@
<artifactId>activemq-all</artifactId>
<version>${project.version}</version>
</dependency>
+ <!-- Remove activemq-camel
<dependency>
<groupId>org.apache.activemq</groupId>
<artifactId>activemq-camel</artifactId>
<version>${project.version}</version>
</dependency>
+ -->
Review comment:
Likewise with these, probably just removal is clearer.
I know this might just be a first "revision", but just in case you forget
##########
File path: activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnectionFactory.java
##########
@@ -271,7 +272,39 @@ public synchronized Connection createConnection(String userName, String password
return newPooledConnection(connection);
}
- protected Connection newPooledConnection(ConnectionPool connection) {
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
+ }
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext(String userName, String password) {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
I like the way you're calling out the specific methods in the exceptions. Makes it clear to diagnose. But you should standardize on having `"createContext(String, String)"` or just `"createContext()"`, and same with the other unimplemented methods.
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
@jbonofre what's the advantage of using geronimo vs. jakarta? I know you mentioned in your implementation you used geronimo - why is one more likely to create conflicts with Camel?
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
This is totally a non-functional style comment, but I feel like every new method you've added has a different indentation level 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelandrepearce commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892962330
It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did.
I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
Sent from my iPad
> On 4 Aug 2021, at 21:29, ehossack-aws ***@***.***> wrote:
>
>
> I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of JMS20CompatibilityTests, that utilize the APIs and act accordingly.
> The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
> A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
>
> As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
>
> That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
> How does that sound? The action items there would be:
>
> @mattrpav to add a series of basic tests that cover the altered APIs
> @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
> (admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that does not throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.
> Triage notifications on the go with GitHub Mobile for iOS or Android.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-880853002
By the way, I'm using Geronimo spec to be aligned with camel and OSGi.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892952534
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of `JMS20CompatibilityTest`s, that utilize the APIs and act accordingly.
The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
How does that sound? The action items there would be:
- @mattrpav to add a series of basic tests that cover the altered APIs
- @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that _does not_ throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892952534
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of `JMS20CompatibilityTest`s, that utilize the APIs and act accordingly.
The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
How does that sound? The action items there would be:
- @mattrpav to add a series of basic tests that cover the altered APIs
- @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that _does not_ throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737003423
##########
File path: activemq-web-demo/pom.xml
##########
@@ -142,8 +142,8 @@
<dependencies>
<!-- j2ee jars -->
<dependency>
- <groupId>org.apache.geronimo.specs</groupId>
- <artifactId>geronimo-jms_1.1_spec</artifactId>
Review comment:
i see that this PR is still using jakarta instead of geronimo , was the mentioned osgi compatibility resolved?, whats the story/consensus, from above it seems that it consensus was to use geronimo but PR isnt updated from that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888507051
As an example, the Qpid JMS has a test module that uses ActiveMQ 5 for some tests and those include a 'foreign message' test. Currently it works with 5.16.2. Modifying the module now to use the snapshot from this PR, it then fails that test due to the change I've noted here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884327669
Up vote for jakarta.jms-api-2.0.3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] rmannibucau commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-893172819
Maybe stupid but in tomee to have jms2 api support i wrapped amq5 in "TomEEAMQ" classes.
This means a first compatibility version can be done in a jms2 package enabling a full jms1 compat in existing package and to start a jms2 support in a new package reusing/delegating to jms1 code.
This is pretty straight forward and fast to do without any regression.
Only change in current code can be in factories with a version config to switch between impl but at the end it is mainly delegation code and works well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] tabish121 commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
tabish121 commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892970530
> It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did. I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
For clarity this has nothing to do with broker side AMQP support, the issue seen from Qpid bridging tests simply exposes an issue with the proposed Openwire client changes which don't represent a complete implementation of the JMS 2.0 specification. It's quite likely that other libraries such as PooledJMS would similarly encounter unexpected situations as it also uses a similar mechanism as the Qpid client does to detect JMS 1.1 clients operating with JMS 2.0 API jars on the classpath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r674156421
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
IDE preferences didn't come over. Fixed in latest commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884327669
Upvote for jakarta.jms-api
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737004876
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
seems in general indentation and code formatting seems out still....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-952419469
seems some code style clean up needs to be done in general before merge, indentation a little over the place in many files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r737003928
##########
File path: activemq-karaf/src/main/resources/features-core.xml
##########
@@ -32,7 +32,6 @@
<bundle dependency='true'>mvn:org.apache.servicemix.bundles/org.apache.servicemix.bundles.jaxb-impl/${jaxb-bundle-version}</bundle>
<bundle>mvn:org.apache.commons/commons-pool2/${commons-pool2-version}</bundle>
<bundle>mvn:commons-net/commons-net/${commons-net-version}</bundle>
- <bundle dependency="true">mvn:org.apache.zookeeper/zookeeper/${zookeeper-version}</bundle>
Review comment:
? why is this being removed as part of jms change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888451870
@gemmellr replies inline..
> @mattrpav I'm not actually all that new to JMS at this stage really, I'm at least a little aware what the methods are for and how the functionality is used, having implemented JMS [2] in full including a full protocol mapping of the functionality in a client capable of doing such scheduled delivery against ActiveMQ 5 for several years now already. I have no idea why you think I was talking about application or 'wrapper' usage of the setter, I wasn't.
Cool
> I think I was pretty clear about taking a 5.x message object and sending it through another JMS 2 client, which is where the issue arises, as it necessarily tries to call setJMSDeliveryTime on every message sent (as either there is a delay, and the actual time needs set, or there isnt and it should be clearing the potentially-non-zero existing value to indicate so). Such a client client may have reasonably handled a JMS 1.1 object not having the method implemented, as e.g. 5.x didnt so far, but it is not going to expect the method to be actually implemented but throw UOE.
Yeah, I'm trying to piece your scenario together, but coming up short.
Am I getting this right:
1. App currently uses JMS 2.0 API
2. App DeliveryDelay is required, and sets DeliveryDelay on all messages through the JMS 2.0 MessageProducer
3. App has a way to handle that ActiveMQ (or other JMS 1.1) doesn't have this method so it doesn't fail
4. Upon upgrade of activemq-client jar, this same unchanged code will now throw an exception
I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem.
As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
> If 5.x is going to use the JMS 2 API then any JMS 2 message object sent with it should reasonably be expected to come back with a delivery time value of 0 after send if there is no delay occurring, i.e it should be doing the equivalent of setJMSDeliveryTime(0) by itself on its own messages, and actually calling setJMSDeliveryTime(0) on other providers messages to clear any existing non-zero value.
I think it is an interesting suggestion-- there is a JIRA open for the DeliveryDelay impl here: https://issues.apache.org/jira/browse/AMQ-8320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892984820
>
> It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did.
They do when updated to prove the issue, but this was caught purely by thinking about it. Showing the test actually fails came later.
>I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
As Tim said, its not really about the full broker side support (Qpid JMS can already do delivery delay against 5.x). This is purely the 5.x Message object breaking the ability to send them through another JMS 2 provider as you are meant to be able, by implementing mandatory header set/get methods that throw an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888484005
>
> Am I getting this right:
>
> 1. App currently uses JMS 2.0 API
>
> 2. App DeliveryDelay is required, and sets DeliveryDelay on all messages through the JMS 2.0 MessageProducer
>
> 3. App has a way to handle that ActiveMQ (or other JMS 1.1) doesn't have this method so it doesn't fail
>
> 4. Upon upgrade of activemq-client jar, this same unchanged code will now throw an exception
>
No. App can use JMS 1.1 or JMS 2.0 API (EDIT: actually yes, the issue will only arise if they use 2.0 to be fair...which would be more likely given its the aim of this change). Configuring DeliveryDelay at all is not required. App sends JMS 1.1 providers message object, through JMS 2.0 client (which tolerates 1.1 messages). After this change as-is, making the '1.1 provider' use the 2.0 API and throw UOE for impl, that will fail all the time during send.
Its possible for a JMS 2.0 client to work upon and send 1.1 message objects, tolerating that the message.setDeliveryTime() method is defined but is not being implemented by the older object (as clearly it didnt exist before, and so couldnt be implemented, but backwards compatibility is the name of the game for JMS). The JMS reference impl definitely did this.
Its possible for a JMS 2.0 client to tolerate being used only with the 1.1 API jar and for the message.setDeliveryTime() method to not actually be defined at all at runtime. I dont recall if the RI did this.
Qpid JMS for example does both of these things. It will let you send a 1.1 message through it, and it will operate using the 1.1 API jar. It simply made sense to support when transitioning specs, not dropping support for using it with existing 1.1 providers (e.g 5.x) or existing applications, when they couldn't actually be doing anything that wasnt still supported as all that API all remained precisely for compatibility. Those are very different situations from the method being defined, implemented and explicitly throwing UOE, and I dont expect anything to really handle that it really makes no sense for it to ever do so, especially for a long setter that is simply for recording 'what actually happened during send'.
Note that all sends will fail, regardless whether a delivery delay is configured on the producer, as a value is always being set on the sent messages to either indicate the actual delivery time or ensure it records there was no delay (as the sent message object may previously have had a deliveryTime value, so that needs cleared to ensure the getter reflects reality).
>
> I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem.
>
The 4th happens because its an entirely different situation than what #3 is handling. A method not existing / not being implemented and that being handled, is quite different than it actually being there but deliberately throwing.
> As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
>
There is no delay unless the feature is implemented (which would remove any need for a discussion), so there cant really be a sillent wiping. The message setters dont control the delay as already discussed, and why would anyone expect the message to be reporting a non-zero delivery time after send, when producer.setDeliveryDelay(..) is throwing to indicate a delay cant actually be configured?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r742268147
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -66,7 +91,7 @@ public void setDisableMessageID(boolean value) throws JMSException {
this.disableMessageID = value;
}
- /**
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
+ }
+
+ /**
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
##########
@@ -42,6 +42,31 @@ public ActiveMQMessageProducerSupport(ActiveMQSession session) {
disableMessageTimestamp = session.connection.isDisableTimeStampsByDefault();
}
+ /**
+ * Gets the delivery delay associated with this <CODE>MessageProducer</CODE>.
+ *
+ * @return this producer's <CODE>DeliveryDely/ <CODE>
+ * @throws JMSException if the JMS provider fails to close the producer due to
+ * some internal error.
+ * @since 2.0
+ */
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ throw new UnsupportedOperationException("setDeliveryDelay() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
+ public JMSContext createContext() {
+ throw new UnsupportedOperationException("createContext() is not supported");
Review comment:
Fixed
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
##########
@@ -285,8 +286,40 @@ public TopicConnection createTopicConnection() throws JMSException {
public TopicConnection createTopicConnection(String userName, String password) throws JMSException {
return createActiveMQConnection(userName, password);
}
+
+ /**
+ * @return Returns the JMSContext.
+ */
+ @Override
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #682:
URL: https://github.com/apache/activemq/pull/682#discussion_r742292509
##########
File path: activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
##########
@@ -307,6 +307,44 @@ public JMSConnectionStatsImpl getConnectionStats() {
/**
* Creates a <CODE>Session</CODE> object.
*
+ * @throws JMSException if the <CODE>Connection</CODE> object fails to
+ * create a session due to some internal error or lack of
+ * support for the specific transaction and acknowledgement
+ * mode.
+ * @since 2.0
+ */
+ @Override
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] jbonofre commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
jbonofre commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-893174170
@rmannibucau yes, it's what I have in mind as well, as a first step (and what I started). @mattrpav took more "ambitious" approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892988374
>
>
> What exactly is breaking? If we put a long in there, it’s a dead store.
It breaks being able to send any 5.x message through any JMS 2 provider that implements what the spec says. Ones which worked before. Breaking.
Its not dead, its needed so that people can use the getter to see the applied value after send. Its specifically what the methods exist for, and it will need to be implemented at some point as its what those methods do. It makes sense to do it now to avoid breaking things.
>
> The pre-openwire v13 change is to map to a property for the interim. See the ticket specifically about DeliveryTime.
>
> My suggestion is to move forward with this set of changes and then I can push the PR w the property-based DeliveryTime solution and this concern goes away as DeliveryTime will be implemented.
This is a local change to the message object, no openwire changes needed. I think it makes sense to do it here, given the discussion, and instead of breaking it first unecessarily then fixing it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-881557987
@jbonofre this change is purely packaging skeleton. I know you mentioned working on full JMS 2.0 support along w/ big things like replicated-kahadb. I thought I'd pitch in to help get the ball rolling on the low-hanging fruit. There is no real code change here, so I think this PR good to merge and should slide in with your real JMS 2.0 impl work. I also broke up the 2.0 Impl work into sub-tasks here-- https://issues.apache.org/jira/browse/AMQ-7309. This should help parallelize the effort amongst contributors.
Reviewing other projects and frameworks, I agree the geronimo vs jakarta is the real question here and I think it makes more sense to go to the Jakarta jar now as it is more target state than using geronimo.
If we go geronimo users will have this to look forward to:
gerinomo 1.1 -> geronimo 2.0 -> jakarta 2.0 -> jakarta 3.0
Alternatively, if we go jakarta now, users have this:
gerinomo 1.1 -> jakarta 2.0 -> jakarta 3.0
CXF has transitioned many spec jars to jakarta, and I suspect Camel will do the same as well. I think the alignment makes more sense for CXF and Camel to consume the dep ActiveMQ (being as ActiveMQ is the JMS provider as those ride on top of ActiveMQ), or they simply provide the <exclude/>. Either way, the JEE transition is going to force framework devs and users into being savvy with Maven <exclude/> and I do not think there is any way around that.
I'd be interested in hearing others' perspective as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888331948
> > @gemmellr - Can you describe this scenario in more detail or a specific use case? I want to make sure we are not breaking anything with throwing unimplemented exceptions and how big of an edge case it is.
>
> Take an ActiveMQ 5 JMS message object, and send that JMS Message somewhere else using a different JMS client. E.g receive from a 5.x broker and publish using the same message object to another type of broker...could be due to communicating with different teams, companies, or handling a system transition, etc.
>
> As part of that process if the other provider is actually a JMS 2 client, it will try to stamp the JMSDeliveryTime on the original message object like its meant to. It may have handled the method not being implemented at all on JMS 1.x message objects since thats not so unexpected due to spec transition, but will likely fail if the method actually exists but simply throws UOE as thats unexpected.
@gemmellr yeah, so tricky corner of the JMS API. This trips up a lot of folks that are new to JMS-- the setDeliveryDelay can only be honored when set on the MessageProducer object. Any code that calls it on the Message is not valid per the spec.
```
7.9 Message Delivery Delay
An application may specify the required delivery delay using the method
setDeliveryDelay on the producer object. This sets the delivery delay of
all messages sent using that producer. Note however that the
setDeliveryDelay method on Message cannot be used to set the
delivery delay of a message.
```
For the JMS bridge idea, I can't walk a scenario where would be _actually_ problematic. This feels hypothetical as it has a chicken-and-egg problem. The JMS API doesn't define how message data is hydrated into the Message object. It is handled by the provider impl and the wire protocol-- there is not support for the DeliveryDelay field in OpenWire at this time and there is nothing in ActiveMQ code base that calls setDeliveryDelay, and then throw an exception unexpectedly on a user.
As far as anyone providing a JMS 2.0 wrapper for ActiveMQ-- any code that is currently calling ActiveMQ using a JMS 2.0 MessageProducer to set a DeliveryDelay must have a custom wrapper impl for MessageProducer. Those scenarios are out-of-reach edge cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888509168
> As an example, the Qpid JMS has a test module that uses ActiveMQ 5 for some tests and those include a 'foreign message' test. Currently it works with 5.16.2. Modifying the module now to use the snapshot from this PR, it then fails that test due to the change I've noted here.
Cool, what is the link to the test code? Test output?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884330168
> Removing the whole of activemq-camel reall seems like something deserving of its own JIRA vs sneaking it into a commit with the title "JMS 2.0 API clean-ups"
@tabish121 agreed. The intent here isn't to 'sneak' it in. A workable solution didn't present itself while making the API change. Before anything is merged, I'll separate out the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-889138211
> > Cool, what is the link to the test code? Test output?
>
> The test code is at https://github.com/apache/qpid-jms/blob/1.1.0/qpid-jms-interop-tests/qpid-jms-activemq-tests/src/test/java/org/apache/qpid/jms/producer/JmsMessageProducerTest.java#L177. Its very simple, 'create foreign 5.x message object, try to send + receive message'. It fails to send on line 200, due to JMSDeliveryTime UOE from the message.
>
> For completeness, this is the change I made to try out the changes: [gemmellr/qpid-jms@516462f](https://github.com/gemmellr/qpid-jms/commit/516462f9d177ab8d1ab48fe29555e464c172201f)
Without a stacktrace, I'm estimating that this is the method throwing the exception:
```
JMSSession.java
private void setForeignMessageDeliveryTime(Message foreignMessage, long deliveryTime) throws JMSException
```
Yeah, so in that code it could be argued that the 'hasDelay' marker variable (used 7 lines earlier in L907) should be used before calling the setJMSDeliveryTime in the private setForeignMessageDeliveryTime method. That method presumes that it should call the setJMSDeliveryTime even if there hasn't been a DeliveryDelay specified.
Either approach (throw UOE or swallow expected Delivery Delays) will present downside to varying users and interests.
I contend that the current UOE approach matches several project goals:
1. The implementation matches the description in the JIRA
2. The mailing list discussion has settled on this approach
3. The desire to keep PR changes concise
There is a dedicated JIRA for DeliveryDelay implentation discussion here: [AMQ-8320](https://issues.apache.org/jira/browse/AMQ-8320)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-884327073
@jbonofre @rmannibucau I've updated this round of the PR to include geronimo-jms for the karaf artifacts. While there is no bigger Karaf fan than me, I think using jakarta as the baseline makes more sense.
_Please here me out on these points below and if you still think we should go geronimo all the way, I'll make the change._
Most users are going to see activemq through the lens of activemq-client and its dependencies:
```
activemq-client
+ jakarta.jms-api-2.0.3
```
vs
```
activemq-client
+ geronimo-jms_2.0_spec-1.0-alpha-2.jar
```
Shortly, we'll have:
```
activemq-client-jms3
+ jakarta.jms-api-3.0.0.jar
```
1. The geronimo part doesn't appear to be a stable release
2. What even is geronimo? How does this fit in with Jakarta? What is a 'spec'?
3. Shortly, we'll want to add a client for Jakarta 3 support. The dependency is going to flip on users again.
4. There is no geronimo-jms for Jakarta 3
5. Optionally, I'm happy to package up jakarta jms api jars as bundles for Karaf (which we may need to do anyway for JMS 3)
Thank you for hearing me out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] michaelpearce-gain commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-960248600
@mattrpav thanks for sorting the formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
mattrpav commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-904634515
> Removing the whole of activemq-camel reall seems like something deserving of its own JIRA vs sneaking it into a commit with the title "JMS 2.0 API clean-ups"
@tabish121 Camel remove PR is here: https://github.com/apache/activemq/pull/701
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] gemmellr commented on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-888910656
> Cool, what is the link to the test code? Test output?
The test code is at https://github.com/apache/qpid-jms/blob/1.1.0/qpid-jms-interop-tests/qpid-jms-activemq-tests/src/test/java/org/apache/qpid/jms/producer/JmsMessageProducerTest.java#L177. Its very simple, 'create foreign 5.x message object, try to send + receive message'. It fails to send on line 200, due to JMSDeliveryTime UOE from the message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] ehossack-aws edited a comment on pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3
Posted by GitBox <gi...@apache.org>.
ehossack-aws edited a comment on pull request #682:
URL: https://github.com/apache/activemq/pull/682#issuecomment-892952534
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of `JMS20CompatibilityTest`s, that utilize the APIs and act accordingly.
The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
How does that sound? The action items there would be:
- @mattrpav to add a series of basic tests that cover the altered APIs
- @gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that _does not_ throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org