[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
GitHub user michaelandrepearce opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1659

    ARTEMIS-1516 - Ensure JNDI via Tomcat Resource works

    Apply fix so that when using JNDI via tomcat resource it works.
    Replace original extract of JNDIStorable taken from Qpid, and use ActiveMQ5's as fits better to address this issue. (which primary use case is users migrating from 5.x)
    Refactored ActiveMQConnectionFactory to externalise and turn into reference by StringRefAddr's instead of custom RefAddr which isnt standard.
    Refactored ActiveMQDestinations similar
    Refactored ActiveMQDestination to remove redundent and duplicated name field and ensured getters still behave the same

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1516

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1659.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1659
   
----
commit 8efb97b15fc7d488a1f371e27ccd3f27b4a33923
Author: Michael André Pearce <[hidden email]>
Date:   2017-11-04T02:26:39Z

    ARTEMIS-1516 - Ensure JNDI via Tomcat Resource works
   
    Apply fix so that when using JNDI via tomcat resource it works.
    Replace original extract of JNDIStorable taken from Qpid, and use ActiveMQ5's as fits better to address this issue. (which primary use case is users migrating from 5.x)
    Refactored ActiveMQConnectionFactory to externalise and turn into reference by StringRefAddr's instead of custom RefAddr which isnt standard.
    Refactored ActiveMQDestinations similar
    Refactored ActiveMQDestination to remove redundent and duplicated name field and ensured getters still behave the same

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    This is PR with Jira and updated commit message, for branch that has been working/validating with @Spaction on already (see discussion there)  https://github.com/michaelandrepearce/activemq-artemis/commit/632f9e389b2562529a3024165d6c9752e965f150
   
    Also we have https://github.com/michaelandrepearce/tomcat-sample showing this as complete end 2 end. (possibly could move this into an examples later in another pr)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @michaelandrepearce this is really a piece of nice work...
   
    Were you able to run it on tomcat... one thing that would be wonderful is an example... or docs...
   
   
    if you could at least tell me what you did.. it would be great.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @cleclebertsuconic as noted we had made  https://github.com/michaelandrepearce/tomcat-sample to example this on tomcat (and for my own piece of mind to prove it) i can move this to examples, though due to personal circumstances won't be able to to that bit till next week (maybe separate PR later using same JIRA?)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @michaelandrepearce sure.. I had missed your URL about the the example. looks great


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151577088
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java ---
    @@ -88,20 +83,6 @@
     
        private boolean finalizeChecks;
     
    -   @Override
    -   public void writeExternal(ObjectOutput out) throws IOException {
    -      URI uri = toURI();
    -
    -      try {
    -         out.writeUTF(uri.toASCIIString());
    --- End diff --
   
    Wouldn't this will break compatibility.
    Think of anyone serializing this class.
   
    Looking at the JNDISTorage (new class) I see that this is calling now writeObject(getProperties());
   
    As opposed to the writeURI.
   
    The point was to write a string to be better compatible between versions.
   
    Can we add back the format we had here?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151581604
 
    --- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/referenceable/SerializableObjectRefAddr.java ---
    @@ -14,7 +14,7 @@
      * See the License for the specific language governing permissions and
      * limitations under the License.
      */
    -package org.apache.activemq.artemis.jms.referenceable;
    --- End diff --
   
    do you really need this move?
   
    I'm thinking about Wildfly integration here.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151581721
 
    --- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/referenceable/ActiveMQRAConnectionFactoryObjectFactory.java ---
    @@ -27,7 +27,7 @@
      *
      * Given a reference - reconstructs an ActiveMQRAConnectionFactory
      */
    -public class ConnectionFactoryObjectFactory implements ObjectFactory {
    +public class ActiveMQRAConnectionFactoryObjectFactory implements ObjectFactory {
    --- End diff --
   
    is that really needed? Thinking of Wildfly again here.
   
    Perhaps an extension?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    The test failure is so weird. if you run the whole thing in Idea it fails.. individually it's passing.
   
   
    I can see it's missing a property.. but I'm not sure what changed that user was at some point considered a property.. as the BeanUtil wasn't picking up builder pattern on setUser... so weird.
   
   
    Anyways.. I sent a PR to your branch.. please squash it.
   
   
    Only thing I'm concerned here is the wire change on the connection factory (as it was a string with the URI before)... and I'm not so sure if you really need to move those classes.. thinking this as an API change since Wildfly will probably need it.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151612096
 
    --- Diff: artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQConnectionFactory.java ---
    @@ -88,20 +83,6 @@
     
        private boolean finalizeChecks;
     
    -   @Override
    -   public void writeExternal(ObjectOutput out) throws IOException {
    -      URI uri = toURI();
    -
    -      try {
    -         out.writeUTF(uri.toASCIIString());
    --- End diff --
   
    No it is serialising fine, in fact this is what tomcat resources is doing, this is exactly what we are purposely changing to fix the issue, it needs to serialise into an array of StringRefAddr (properties).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151612267
 
    --- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/referenceable/SerializableObjectRefAddr.java ---
    @@ -14,7 +14,7 @@
      * See the License for the specific language governing permissions and
      * limitations under the License.
      */
    -package org.apache.activemq.artemis.jms.referenceable;
    --- End diff --
   
    no we didn't, but its only applicable to RA which has its own module we can make that package in the RA module.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151612327
 
    --- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/referenceable/ActiveMQRAConnectionFactoryObjectFactory.java ---
    @@ -27,7 +27,7 @@
      *
      * Given a reference - reconstructs an ActiveMQRAConnectionFactory
      */
    -public class ConnectionFactoryObjectFactory implements ObjectFactory {
    +public class ActiveMQRAConnectionFactoryObjectFactory implements ObjectFactory {
    --- End diff --
   
    was a tidy up, again not needed but just being explicit. we can rename and extend.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    The serialisation change was actually needed for the fix, this is some of the actual issue.
   
   
    > On 17 Nov 2017, at 04:17, clebertsuconic <[hidden email]> wrote:
    >
    > The test failure is so weird. if you run the whole thing in Idea it fails.. individually it's passing.
    >
    > I can see it's missing a property.. but I'm not sure what changed that user was at some point considered a property.. as the BeanUtil wasn't picking up builder pattern on setUser... so weird.
    >
    > Anyways.. I sent a PR to your branch.. please squash it.
    >
    > Only thing I'm concerned here is the wire change on the connection factory (as it was a string with the URI before)... and I'm not so sure if you really need to move those classes.. thinking this as an API change since Wildfly will probably need it.
    >
    > —
    > You are receiving this because you were mentioned.
    > Reply to this email directly, view it on GitHub <https://github.com/apache/activemq-artemis/pull/1659#issuecomment-345115665>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUtLgcu7LPIG2FuPTUm4ALiIxQn_CVrks5s3N6kgaJpZM4QhAPN>.
    >
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat R...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1659#discussion_r151617383
 
    --- Diff: artemis-ra/src/main/java/org/apache/activemq/artemis/ra/referenceable/SerializableObjectRefAddr.java ---
    @@ -14,7 +14,7 @@
      * See the License for the specific language governing permissions and
      * limitations under the License.
      */
    -package org.apache.activemq.artemis.jms.referenceable;
    --- End diff --
   
    Ill add back like you mention in the other comment a back compatibility extension class in the old location.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    The serialisation change was actually needed for the fix, this is some of the actual issue.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @michaelandrepearce I will try to make it compatible with Wildfly and previous versions next week.
   
    We may need to add new classes if it' snot possible at all. Extensions...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @clebertsuconic see latest (pushed the other day) I had added classes / packages to keep the compatibility


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @michaelandrepearce What about serialization? you still changed it.. didn't you?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @clebertsuconic managed to solve that also so it can keep existing


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1659: ARTEMIS-1516 - Ensure JNDI via Tomcat Resource...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1659
 
    @clebertsuconic i have added a document page now, and also contributed in the example i had separately into the examples section.


---
12