[GitHub] activemq-artemis pull request #1695: ARTEMIS-1545 Ensure JMS security except...

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

[GitHub] activemq-artemis pull request #1695: ARTEMIS-1545 Ensure JMS security except...

pgfox
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-1545 Ensure JMS security exceptions occur if NON-PERSISTENT

    Add test case to ensure exception behaviour on JMS MessageProducer send is the same, if message is sent persistently or non-persistently when using default settings.
    Update default setting to ensure behaviour is the same, as per expectation when using JMS by default. (e.g. no setting overrides)

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

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

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

    https://github.com/apache/activemq-artemis/pull/1695.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 #1695
   
----
commit 4bf4c9ba02fd85f01fcb958ae5bf4e27cb4aefc1
Author: Michael André Pearce <[hidden email]>
Date:   2017-12-07T23:07:40Z

    ARTEMIS-1545 Ensure JMS security exceptions occur if NON-PERSISTENT
   
    Add test case to ensure exception behaviour on JMS MessageProducer send is the same, if message is sent persistently or non-persistently when using default settings.
    Update default setting to ensure behaviour is the same, as per expectation when using JMS by default. (e.g. no setting overrides)

----


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

pgfox
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1695
 
    I'm not sure we'd want to change the default here as it will have significant performance implications.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    How would you solve this issue then? On testing performance in our system (it’s quite high perf) this really didn’t have any impact (if anything it reduced latency)


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    Eg out the box you expect JMS behaviour to be correct that exceptions are thrown in these cases and likewise if unable to send to the broker it throw me exception. Behaviour should be the same if persistent or not, the difference only in no persistence is about if the message should be persistent during broker restart/failure


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    On note of perf it should note that it doesn’t stop someone setting it to true, it just makes the behaviour by default (eg expectation) to be correct, which is most important especially with adopting / migrating.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    I think expected JMS behaviour is more important.
   
    Persistence is about if the message should be persistent during broker restart/failure for broker side perfomance. It shouldn't change the behaviour of the client in regards to exceptions.
   
    Like wise as a user you would expect client side exception by default (spec or no-spec).
   
    When checking other vendors (We checked out other two brokers) and also 5.x, exception on send to the broker is thrown back to the client with non-persistent. (e.g. no difference between persistent or not in these case)
   
    I think this is particularly important for anyone migrating (and pertinent with regards to 5.X users moving) that they would still get JMS exceptions on send and not get any surprises that they don't get them.
   
    FYI this is what we are doing, migrating from several different brokers onto Artemis.
   
    On the note of performance (as you may be aware or not) we do care for performance (throughput and latency) in our brokers due to nature of the business, but expected behaviour (aka no shocks) is more important. On that note we actually did do some tests and found produce to consume latency reduced per message.
   
    Like wise for those really wanting it and willing to sacrifice no client side exceptions they still can set this to false consciously, but the point is the default should be that users do expect them, and probably should get such surprises.
   
     
   



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    I agree with Justin, I think its a bad idea to change this. we should find a different solution if we want to propagate exceptions back to the client.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    @andytaylor to propagate exceptions on send (with it being sync) it will have to block somewhere, and wait for the return packet, i don't see much other options. It be good if there is one you have in mind if you could share.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    @michaelandrepearce I dont have one :), however I dont see a reason to change defaults that have around for such a long time.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    @andytaylor the issue i see is that the defaults should err on the side of safety imo.
   
    Having a client not throw any exception and continue blindly is dangerous (even if its miss-config). Just because something is like it is, doesn't always been it should be.
   
    I get the performance thing, but then thats something someone should make a conscious decision to enable at the expense of exceptions.
   
    Just a bit of background, we migrated a flow (it was using JMS api, so switch is just in the CF) from another vendor's broker to Artemis, and released to prod.
   
    The broker did alert, but the operations team operating the broker saw it as a client issue. and as was a client issue they believed the client team would also get an error, have been alerted and would have resolved.
    Because the client team had no error, it was not until quite a period later it was discovered and external reports had to alert us to the issue.
   
    We re-tested the scenario with our other brokers, and they would have alerted client side so this is why this assumption is had.
   
    If we look to other people migrating I'm fairly sure the other brokers we use, are quite well used, and as such other users would make similar assumptions and expectation that the client would still error by default, unless they toggled something for performance on purpose.
   



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    <Copied from the JIRA since we apparently don't read those comments any more>
   
    This doesn't actually strike me as being a bug on the send as many JMS provider will indeed send the message asynchronously and if any error is reported on send fail it'd be done via the JMS ExceptionListener on the Connection. It would be nice if the createProducer failed here if not authorized to write to the target destination but if using an anonymous producer than again this seems to operate as many other JMS clients would.
   
    The standard practice here would be to use a transaction to create a sync point (i.e. the commit) where the broker would report the operation as failed if any of the sends inside that TX failed for some reason. Since the NON_PERSISTENT delivery mode is 'at-most-once' you shouldn't expect the same QoS behaviors on the client send (unless of course you can configure that which appears you could via the block option).



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    That’s true if you use the JMS 2.0 async send with an on completion listener. But this is older 1.1 sync send method.
   
    Note that also there is an issue with the async send also in artemis client currently as onException is never called only onAcknowledge as such cannot make use of that due to that bug/issue.
   



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    Eg if that worked then we could make use of that here  to get the exception without blocking send.
   



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    Actually I was mostly referring to JMS 1.1 sends where many JMS client opt to send non-persistent messages async and indeed won't report an error back unless you've explicitly configured the client to do so.



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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    So I think we need the core layer to handle back exceptions it’s callback only seems be success only, anyone got any pointers where to start and how we should look to get that so it gets errors back async to pass to the callback


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    I'm not compelled by the "expectations" argument because different users have different expectations.  Users from one JMS implementation might have an expectation that non-persistent messages are sent synchronously, but any user coming from HornetQ, Artemis, or (more importantly, I think) ActiveMQ 5.x  among others would have the opposite expectation.  This behavior is not defined by the spec, and therefore should be well documented so users will be informed and can change their expectations (if necessary).
   
    I think the best way forward would be to improve the SendAcknowledgementHandler feature in Artemis which serves as the basis for the JMS CompletionHandler implementation.


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    @tabish121 this isn’t what we found on testing our two other incumbent brokers. There is a toggle on one of them to make it not wait and no exceptions but you have to actively override the default


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    @jbertram I think that’s where I got to also, just don’t know where to start... any advice or tips?


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    THe last comment was re SendAcknowledgementHandler  improving


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

[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...

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

    https://github.com/apache/activemq-artemis/pull/1695
 
    At this point I don't even think the broker is even sending anything back.  It looks to me like the client simply infers success based on no exceptions when pushing the message into the transport layer.


---
12