[GitHub] [activemq] ehossack-aws commented on a change in pull request #682: [AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3

[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