You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/03/15 15:00:49 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
gtully opened a new pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
This is the same as when we aligned queue naming for amqp, so that it used the same rules as core and openwire so there wasnt a conflict and you could then have openwire core and amqp consumers share the same shares subs (queues). Whilst it would have been nice to default on the better new behaviour, to avoid least astonishment for existing users it went behind a toggle and defaulted to the original behaviour.
Only at the next major release aka 3.0.0 would we then be able to change that default behaviour (break change) with the major version change. Theres a few things like the one i mentioned thats waiting to change those defaults at the next major.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
This is the same as when we aligned queue naming for amqp, so that it used the same rules as core and openwire so there wasnt a conflict and you could then have openwire core and amqp consumers share the same shares subs (queues). Whilst it would have been nice to default on the better new behaviour, to avoid least astonishment for existing users it went behind a toggle and defaulted to the original behaviour.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
This is the same as when we aligned queue naming for amqp, so that it used the same rules as core and openwire so there wasnt a conflict. Whilst it would have been nice to default on the better new behaviour, to avoid least astonishment for existing users it went behind a toggle and defaulted to the original behaviour.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
This is the same as when we aligned queue naming for amqp, so that it used the same rules as core and openwire so there wasnt a conflict and you could then have openwire core and amqp consumers share the same shares subs (queues). Whilst it would have been nice to default on the better new behaviour, to avoid least astonishment for existing users it went behind a toggle and defaulted to the original behaviour.
Only at the next major release aka 3.0.0 would we then be able to change that default behaviour with the major version change. Theres a few things like the one i mentioned thats waiting to change those defaults at the next major.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594596619
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -875,7 +936,7 @@ public void testMax() throws Exception {
underTest.put(new SimpleString("#.a"), new SimpleString("#.a"));
assertEquals(3, countMatchingWildcards(new SimpleString("test.a")));
- assertEquals(1, countMatchingWildcards(new SimpleString("test.a.a")));
+ assertEquals(3, countMatchingWildcards(new SimpleString("test.a.a")));
Review comment:
Again this shouldn't change
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original.
----------------------------------------------------------------
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-artemis] sebthom commented on pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
sebthom commented on pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#issuecomment-799520321
I can confirm that the changes solve the issue we are facing with Artemis 2.0.17.
----------------------------------------------------------------
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-artemis] gtully commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594979079
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
@michaelandrepearce this is a bug fix for a regression, the tests verify the behaviour of the existing code but the semantic was wrong in the tests.
----------------------------------------------------------------
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-artemis] sebthom commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594607740
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -650,7 +650,7 @@ public void testHashA() throws Exception {
SimpleString AHashA = new SimpleString("a.#.a");
underTest.put(AHashA, AHashA);
- assertEquals(1, countMatchingWildcards(new SimpleString("a.b.c.a")));
+ assertEquals(2, countMatchingWildcards(new SimpleString("a.b.c.a")));
Review comment:
Here "#.a" and "a.#.a" are matched against "a.b.c.a". Both should match, given that as per doc "# match any sequence of zero or more words"
----------------------------------------------------------------
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-artemis] gtully merged pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
gtully merged pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594595602
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -650,7 +650,7 @@ public void testHashA() throws Exception {
SimpleString AHashA = new SimpleString("a.#.a");
underTest.put(AHashA, AHashA);
- assertEquals(1, countMatchingWildcards(new SimpleString("a.b.c.a")));
+ assertEquals(2, countMatchingWildcards(new SimpleString("a.b.c.a")));
Review comment:
Ditto comment i would expect existing test case to return the same
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
This is the same as when we aligned queue naming for amqp, so that it used the same rules as core and openwire so there wasnt a conflict and you could then have openwire core and amqp consumers share the same shares subs (queues). Whilst it would have been nice to default on the better new behaviour, to avoid least astonishment for existing users it went behind a toggle and defaulted to the original behaviour.
Only at the next major release aka 3.0.0 would we then be able to change that default behaviour (break change) with the major version change. Theres a few things like the one i mentioned thats waiting to change those defaults being switched to default true at the next major.
----------------------------------------------------------------
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-artemis] sebthom commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594606336
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
Isn't this test essentially about if "#.a" matches "a.b.c.a"? Which I think it should.
----------------------------------------------------------------
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-artemis] gtully commented on pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#issuecomment-799547254
thanks for verification. I should have a clean test run in a few hours and can push this fix.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594596619
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -875,7 +936,7 @@ public void testMax() throws Exception {
underTest.put(new SimpleString("#.a"), new SimpleString("#.a"));
assertEquals(3, countMatchingWildcards(new SimpleString("test.a")));
- assertEquals(1, countMatchingWildcards(new SimpleString("test.a.a")));
+ assertEquals(3, countMatchingWildcards(new SimpleString("test.a.a")));
Review comment:
Again this shouldn't change
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594827965
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
My issue is that would be a change of behaviour, that some users may have got used to and built systems around such quirks. As such just changing that may break many users.
If theres a change in behaviour it needs to be behind a toggle and then by default kept as original. And original behaviour tests kept as is to ensure that.
----------------------------------------------------------------
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-artemis] michaelandrepearce commented on a change in pull request #3492: ARTEMIS-3180 - fix multiple path match case in wildcard address map
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3492:
URL: https://github.com/apache/activemq-artemis/pull/3492#discussion_r594595227
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/postoffice/impl/AddressMapUnitTest.java
##########
@@ -638,7 +638,7 @@ public void testHashA() throws Exception {
assertFalse(aABCA.matches(aHashA));
assertFalse(aHashA.matches(aABCA));
- assertEquals(0, countMatchingWildcards(abcaS));
+ assertEquals(1, countMatchingWildcards(abcaS));
Review comment:
This doesnt seem right. I would expect existing test case to pass the same without modification
----------------------------------------------------------------
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