[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 I am merging your changes.. however I'm reverting the portion of the version properties check. we may revisit this later if you see any other issues. To make eventual cherry-picking into 2.6.1 easier.. I'm making this a single commit. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Ill close this if you merge yours, im happy with yours. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 the more I digg.. the more I'm convinced there's no need for the version check... if we can speak on IRC before we merge this? The version check was basically for wire compatibility... for semantic you just change the server... there's no wire changes here. .just keep it this way? and if this is breaking the client->server contract in a way it needs a raise to the properties. .this would bump the release into 2.7.0... We can't make changes to versioning on point releases... if later on we need to make any other changes into 2.7.0... it will be some extra complexity on bumping the release there again. It seems we can simplify this a lot? ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 compatibility tests are passing with this.. do you know any cases where this would brea compatibility and the current tests have missed it? if that's the case we need to add a test on the compatibility... ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 that would mean older clients will still hit the bug if you keep the version check ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @michaelandrepearce Maybe I'm being slow.. but why do you need to check for version here? shouldn't this be a simple fix on checking the address before? from what I udnerstood from the fix is that you always remove the queue when creating a shared subscription? ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 I took a day off after the release:) If you are happy with it feel free to merge it. We can amend later if anything is failing. Or I can merge later tonight or tomorrow morning. â typing on the iPhone. From a beach. :) ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 This looks good to me. @clebertsuconic, what do you think? ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @jbertram fixed checkstyle introduced in last change, to address franz's review comment. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user jbertram commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Looks like a checkstyle violation. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Should be sorted now. Added checks for address and filterstring at along side checking the queue exists. Ive run locally SharedConsumerTest you mentioned and with this it now passes. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Ill have a look i think i know what it is (need to add filter check at same point as exists check and should be super simple. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 These following tests are failing: Test Result (14 failures / +14) org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryQueueNotAuthorized org.apache.activemq.artemis.tests.integration.amqp.JMSConnectionWithSecurityTest.testCreateTemporaryTopicNotAuthorized org.apache.activemq.artemis.tests.integration.jms.consumer.JmsConsumerTest.testValidateExceptionsThroughSharedConsumers org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelector org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelector org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentTopic org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorSrcFilterNull org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorSrcFilterNull org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedDurableSubOnDifferentSelectorTargetFilterNull org.apache.activemq.artemis.tests.integration.jms.jms2client.SharedConsumerTest.sharedNonDurableSubOnDifferentSelectorTargetFilterNull These tests also failed when I ran but they are probably part of the intermittent failures: org.apache.activemq.artemis.protocol.amqp.util.CreditsSemaphoreTest.testDownAndUp org.apache.activemq.artemis.tests.integration.cluster.failover.ReplicatedLargeMessageWithDelayFailoverTest.testFailThenReceiveMoreMessagesAfterFailover2 org.apache.activemq.artemis.tests.integration.cluster.reattach.MultiThreadRandomReattachTest.testD org.apache.activemq.artemis.tests.integration.cluster.reattach.NettyMultiThreadRandomReattachTest.testE (Although I believe CreditsSemaphoreTest failed because of these changes.. I'm not 100% sure) ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 I'm running the testsuite first before merging this... ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @michaelandrepearce Yep, makes sense indeed. I will probably implemented it in the same you've done (on Core). I haven't finished to read the AMQP part yet :+1: ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @franz1981 non durable also needs it. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user franz1981 commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @michaelandrepearce it looks fine to me: probably I would have performed the check about the queue existence just for the durable ones ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @clebertsuconic email is on its way. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Can you bring this up on the voting thread? ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @clebertsuconic well the bug is been there for ages, just more evident/exposed since 2.5.0 where security check was corrected, so made it more apparent. If its easy to re-spin 2.6.0 yes i would probably re-spin it. It is a blocker for my org, as pre-2.5.0 with the security we have as the bug in security hid the bug in client code, so i would imagine it could be a blocker for other users. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 Wouldnât this make a blocker for 2.6.0? Or this is not that critical ? Iâm reading on the iPhone with limited visibility of the changes. But it seems to warrant a -1 on the current release vote. Let me know please. ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 @andytaylor ---
[GitHub] activemq-artemis issue #2093: ARTEMIS-1872 Check for queue exists before cre...
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/2093 See for full description of the problem, or see test case. https://issues.apache.org/jira/browse/ARTEMIS-1872 ---
