[GitHub] [activemq] jgallimore opened a new pull request #656: AMQ-8277 Copy consumerinfo (removing the additional predicate) before…

[GitBox <gi...@apache.org>]

jgallimore opened a new pull request #656:
URL: https://github.com/apache/activemq/pull/656


   … sending the advisory message to trigger the subscription over the network of brokers.
   
   If a consumer uses an additionalPredicate (as part of a plugin, for example to filter messages to the consumer), the additionalPredicate will cause a ClassCastException and messages will not be forwarded across the network for this consumer.
   
   This PR creates a copy of the ConsumerInfo object (which does *not* copy the additional predicate), and uses the copy of the ConsumerInfo object to send the advisory message to trigger the subscription across the network.
   
   One (potential) drawback here is that the subscription between the brokers is does not filter the messages. 
   
   I did attempt to add this, by making a wrapper that implements both BooleanExpression and DataStructure, and could be marshalled into OpenWire, and then checking it in NetworkBridgeFilter. This did work, but appeared to introduce a number of regressions across the test suite. I'm very happy to take a another swing at this approach if it is preferred.
   
   I have run the full test suite across this PR, and it doesn't appear to break any other tests on my setup.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq] cshannon commented on pull request #656: AMQ-8277 Copy consumerinfo (removing the additional predicate) before…

[GitBox <gi...@apache.org>]

cshannon commented on pull request #656:
URL: https://github.com/apache/activemq/pull/656#issuecomment-846975779


   The main issue I see here is what you pointed out...while this prevents the error it would result in not filtering between brokers may result in unintended behavior which isn't great either and the only real way to fix it is to make the predicate be included by OpenWire (maybe another version or what you already tried)
   
   For the topic scenario (and using conduit subscriptions) it would work fine as selectors are not used anyways as it is pub/sub and messages are just broadcast so when the messages hit the different brokers they can be filtered there.
   
   When using a queue with multiple consumers I would think this would lead to stuck messages. For example a scenario might be if you had 3 brokers (A, B, C) and you create consumers on A and B with different predicates but those predicates are now filtered out when creating demand with advisories. That means if a message is published to broker C that could would match the predicate on consumer on A but not B it may still be forwarded to B and now stuck.
   
   I'm not really sure how much it matters as I doubt many people are doing this as this issue would have been brought up more often. This fix would probably be ok as long as we update the documentation on the network of brokers page and add something that specifically points out that if that field is set a network of brokers ignores 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [activemq] cshannon edited a comment on pull request #656: AMQ-8277 Copy consumerinfo (removing the additional predicate) before…

[GitBox <gi...@apache.org>]

cshannon edited a comment on pull request #656:
URL: https://github.com/apache/activemq/pull/656#issuecomment-846975779


   The main issue I see here is what you pointed out...while this prevents the error it would result in not filtering between brokers and may cause unintended behavior which isn't great either and the only real way to fix it is to make the predicate be included by OpenWire (maybe another version or what you already tried)
   
   For the topic scenario (and using conduit subscriptions) it would work fine as selectors are not used anyways as it is pub/sub and messages are just broadcast so when the messages hit the different brokers they can be filtered there.
   
   When using a queue with multiple consumers I would think this would lead to stuck messages. For example a scenario might be if you had 3 brokers (A, B, C) and you create consumers on A and B with different predicates but those predicates are now filtered out when creating demand with advisories. That means if a message is published to broker C that could would match the predicate on consumer on A but not B it may still be forwarded to B and now stuck.
   
   I'm not really sure how much it matters as I doubt many people are doing this as this issue would have been brought up more often. This fix would probably be ok as long as we update the documentation on the network of brokers page and add something that specifically points out that if that field is set a network of brokers ignores 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org