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 2020/06/24 12:24:19 UTC
[GitHub] [activemq-artemis] jsmucr opened a new pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
jsmucr opened a new pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201
This PR adds the ability to undeploy diverts by removing them from the `broker.xml` configuration file.
----------------------------------------------------------------
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] jsmucr commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448347263
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@clebertsuconic I'll keep this in mind and try to resolve it later.
Your PR resolves this by reverting part of the functionality that I've implemented. It does not fix the problem as a whole.
For now I think it would be for the best to revert #3172 so neither the build or the configuration mechanism is broken. I shall come with a different approach later.
----------------------------------------------------------------
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] clebertsuconic commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448336678
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@jsmucr if you could check and fix RedeployTest::testUndeployDivert and testRedeployQueueDefaults
with my latest PR merged
org.apache.activemq.artemis.tests.integration.client.UpdateQueueTest.testUpdateQueueWithNullUser is passing.
But as you and Michael said something else seems to be going on.
I won't release 2.14 for another week, so we have time to fix this.
----------------------------------------------------------------
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] jsmucr commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448117467
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@clebertsuconic @michaelandrepearce Thank you for the review, I'll get on with 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 #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448014345
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@clebertsuconic re the user in other pr, i think the issue is the test that failed was enforcing you cannot clear a user, that test probably should have been changed. @jsmucr will have to send a pr tomorrow to revert your change and fix the test so that its legal to clear a user. That said if you need to release it can be sorted in separate ticket.
Re this one on diverts if @jsmucr doesnt get back to you in 24hrs, revert the PR
----------------------------------------------------------------
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] jsmucr commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
jsmucr commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448332109
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
\*sad face\*
My PR is wrong. What I originally wanted to implement was to reset the values when reading `broker.xml`. What I've done instead is that it resets the queue configuration every time an incomplete configuration object is passed to the `updateQueue` method. Which in fact breaks the `testUpdateQueueWithNullUser` test.
----------------------------------------------------------------
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] clebertsuconic commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448337980
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
I have no idea why testUndeployDivert fails... when I run it on my laptop it always pass. when I run it as part of mvn -Ptests test, it fails most of the time. (9 in 10). so I have not been successful on debugging it.
if you have no idea I will probably create a branch with lots of traces.
(perhaps the collector from your change fails with ConcurrentModificationException and redeployment does not work?) that's the only theory I have but I tried to prove it and I couldn't.
----------------------------------------------------------------
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] michaelpearce-gain commented on pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#issuecomment-649069987
@jsmucr thanks again for the contributions.
----------------------------------------------------------------
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 #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448014345
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@clebertsuconic re the user in other pr, i think the issue is the test that failed was enforcing you cannot clear a user, that test probably should have been changed. @jsmucr will have to send a pr to revert your change and fix the test so that its legal to clear a user. That said if you need to release merge your pr it can be sorted in separate release/ticket
Re this one on diverts if @jsmucr doesnt get back to you in 24hrs, revert the PR
----------------------------------------------------------------
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 #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r448014345
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@clebertsuconic re the user in other pr, i think the issue is the test that failed was enforcing you cannot clear a user, that test probably should have been changed. @jsmucr will have to send a pr tomorrow to revert your change and fix the test so that its legal to clear a user. That said if you need to release it can be sorted in separate ticket.
Re this one on diverts if @jsmucr doesnt get back to you, revert the PR
----------------------------------------------------------------
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] clebertsuconic commented on a change in pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#discussion_r447989516
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
##########
@@ -448,6 +450,63 @@ public void testRedeployQueueDefaults() throws Exception {
}
}
+ @Test
+ public void testUndeployDivert() throws Exception {
Review comment:
@michaelpearce-gain / @jsmucr any chance you can look at why this test is failing?
if you run it as part of the test suite (mvn -Ptests test) it breaks most of the time.
And this is blocking me from releasing.
Another point is the clash of the user redeployment as it's clashing with a previous definition.. see my PR.
----------------------------------------------------------------
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] clebertsuconic commented on pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201#issuecomment-651496039
@michaelpearce-gain / @jsmucr there was a test broken after this PR, and I'm fixing it on #3209
however the test itself clash with the semantic you're presenting here. so I need you to review the change. and if the semantics are clashing we will need to address it.
thanks
----------------------------------------------------------------
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] asfgit closed pull request #3201: ARTEMIS-2820 Undeploy diverts by removing them from broker.xml
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3201:
URL: https://github.com/apache/activemq-artemis/pull/3201
----------------------------------------------------------------
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