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/10/19 05:00:33 UTC
[GitHub] [activemq-artemis] nbrendah opened a new pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
nbrendah opened a new pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801
I have tried to refactor the ActiveMQServer method replay(address, .....) on the ActiveMQServerControl such it should just call server.replay(this.address, .....)
cc @clebertsuconic
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733709420
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlTest.java
##########
@@ -580,6 +591,107 @@ public void testPurge() throws Exception {
Wait.assertEquals(0L, () -> addressControl.getMessageCount(), 2000, 100);
}
+ @Test
+ public void testReplayWithoutDate() throws Exception {
+ testReplaySimple(false);
+ }
+
+ @Test
+ public void testReplayWithDate() throws Exception {
+ testReplaySimple(true);
+ }
+
+ private void testReplaySimple(boolean useDate) throws Exception {
+ ActiveMQServerControl serverControl = createManagementControl();
Review comment:
the point I made about copying, wasn't to be literal :)
you have to use the ActiveMQAddressControl on this... you are not testing your new code, you are testing the old code on this case.
Everything else seems spot on.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah edited a comment on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah edited a comment on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948234857
Thank you @clebertsuconic for taking time to look at this.
I have updated the PR to match the suggestions and comments above. If you get some free time, i request for a review.
Thanks for the good work
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733869721
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlTest.java
##########
@@ -580,6 +590,111 @@ public void testPurge() throws Exception {
Wait.assertEquals(0L, () -> addressControl.getMessageCount(), 2000, 100);
}
+ @Test
+ public void testReplayWithoutDate() throws Exception {
+ testReplaySimple(false);
+ }
+
+ @Test
+ public void testReplayWithDate() throws Exception {
+ testReplaySimple(true);
+ }
+
+ private void testReplaySimple(boolean useDate) throws Exception {
+ SimpleString address = RandomUtil.randomSimpleString();
+
+ AddressControl addressControl = createManagementControl(address);
+ String queue = "testQueue" + RandomUtil.randomString();
+ server.addAddressInfo(new AddressInfo(queue).addRoutingType(RoutingType.ANYCAST));
+ server.createQueue(new QueueConfiguration(queue).setRoutingType(RoutingType.ANYCAST).setAddress(queue));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("core", "tcp://localhost:61616");
+ try (Connection connection = factory.createConnection()) {
+ Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
Review comment:
I don't mind about the Session duplicated really.. all I mind is you using the AddressControl
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733303390
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/AddressControl.java
##########
@@ -230,4 +230,15 @@ String sendMessage(@Parameter(name = "headers", desc = "The headers to add to th
@Operation(desc = "Purges the queues bound to this address. Returns the total number of messages purged.", impact = MBeanOperationInfo.ACTION)
long purge() throws Exception;
+ @Operation(desc = "Makes the broker to read messages from the retention folder matching the address and filter.", impact = MBeanOperationInfo.ACTION)
+ void replay(@Parameter(name = "address", desc = "Name of the address to replay") String address,
Review comment:
the address should not be a parameter on the AddressControl...
That's the purpose, you understand? you delegate the replay from AddressControl calling ActiveMQServer
that way you pass in (target, filter) on the AddressControl,
and the implementation will call Server.replay(this.address, target, filter);
This would make it simpler, because you are usually browsing messages at the address level on the console. it would make it simpler for users.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic edited a comment on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948241083
also, next time you mvn build, use the -Pdev profile at least once.. it takes some extra time, but it will run the check style.. etc...
the -Derrorprone may also help with the PR checks to make sure it passes it.
```
mvn -Pdev install -Derrorprone
```
One thing I do all the time:
```
mvn -Pdev install -DskipTests=true -Derrorprone
```
I actually have an alias on my OS to those scripts...
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733303476
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +576,22 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay(String address, String target, String filter) throws Exception {
Review comment:
remove String.address from here
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948843931
@nbrendah congratulations.. you got your first PR merged into Artemis :)
I had one small tweak that I almost squashed into your changes... I decided to keep it separated so you could see the small changes I had to apply...
your task is now complete for the outreachy.org program... please, submit your completion and if you have any questions contact me
but feel free to keep contributing to us if you like :)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733273289
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +574,11 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay() throws Exception {
Review comment:
Thanks @clebertsuconic
> I think you the signature should be the same one on ServerControl minus the address...
`server.replay(null, null, this.getAddress(), null, null);`
In my thinking, referencing the `this is trivial.` This is what i mean,
```
public void replay(String address, String target, String filter) throws Exception {
server.replay(null, null, this.address, target, filter);
}
```
Or i should just ignore the `this`, for now?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah edited a comment on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah edited a comment on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948269393
@clebertsuconic
I honestly cant thank you enough, but i appreciate the time you have invested in this.
I have added more changes.
I hope its fine now.
Have a good night.
> `mvn -Pdev install -Derrorprone`
This is a good command because it even checks style guide conventions.
I received a build success locally. Let me wait for that on upstream
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733743150
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
Hey @clebertsuconic
I think the method `proxy.invokeOperation` calls AddressControl.replay(.....) so there is no need of passing the `this.getAddress()` as an argument
ie
```
@Override
public void replay(String target, String filter) throws Exception {
proxy.invokeOperation("replay", this.getAddress(), target, filter);
}
```
Am sorry, it seams am taking too much time on the would be simplest issues
I am just not familiar with this code base. Am sure with some few days, my speed will boost.
Thanks once again
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r732523600
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
Hello @clebertsuconic
Thank you so much for making this more clear. All existing tests are passing but i am out of opinions on how to create tests match this logic.
Thank you so much.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733303390
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/AddressControl.java
##########
@@ -230,4 +230,15 @@ String sendMessage(@Parameter(name = "headers", desc = "The headers to add to th
@Operation(desc = "Purges the queues bound to this address. Returns the total number of messages purged.", impact = MBeanOperationInfo.ACTION)
long purge() throws Exception;
+ @Operation(desc = "Makes the broker to read messages from the retention folder matching the address and filter.", impact = MBeanOperationInfo.ACTION)
+ void replay(@Parameter(name = "address", desc = "Name of the address to replay") String address,
Review comment:
the address should not be a parameter on the AddressControl...
That's the purpose, you understand? you delegate the replay from AddressControl calling ActiveMQServerControl
that way you pass in (target, filter) on the AddressControl,
and the implementation will call Server.replay(this.address, target, filter);
This would make it simpler, because you are usually browsing messages at the address level on the console. it would make it simpler for users.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948234857
Thank you @clebertsuconic for taking time to look at this.
I have added updated the PR to match the suggestions and comments above. If you get some free time, i request for a review.
Thanks for the good work
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948884825
Thank you @clebertsuconic for all the assistance and help on this.
I have seen the tweaks you did https://github.com/apache/activemq-artemis/commit/fdc0cc591c9ab9dea22a9b65cef6c53ea0263222
I would wish to continue contributing Artemis. Actually, i had even created https://github.com/apache/activemq-artemis/pull/3805 to learn more. Maybe, if you get some free time, I request you to have a look.
I think time i am not going to be too much of a slow learner :laughing:
Thank you so much
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948240642
@nbrendah Nice job... you're getting there...It's midnight here and I made sure I came to look, because it looked almost good already...
a few minor adjustments and it will be ready to merge.. not bad! pretty much you have to remove the address parameter from the address control (the address control knows the address name already, that's the point), which will be delegated to the server's with the this.addressName from the address controller (I don't remember the actual variable name, but it will be there somehow).
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948241083
also, next time you mvn build, use the -Pdev profile at least once.. it takes some extra time, but it will run the check style.. etc...
the -Derrorprone may also help with the PR checks to make sure it passes it.
```
mvn -Pdev install -Derrorprone
```
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733303476
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +576,22 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay(String address, String target, String filter) throws Exception {
Review comment:
remove "String address" from here (as I mentioned on the interface declaration)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948269393
@clebertsuconic
I honestly cant thank you enough, but i appreciate the time you have invested in this.
I have added more changes.
I hope its fine now.
Have a good night.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733852073
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
@clebertsuconic
I have pushed some changes.
All tests are running locally,
> Try to understand the functionality a little bit... if you like I can make a video call at some point with you and some other people.
Answering the very many questions that rise as i am working on this issue is what pull be behind somewhat. But this is just temporally, because of the familiarity i have with this code-base workflow.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733303561
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +576,22 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay(String address, String target, String filter) throws Exception {
+ server.replay(null, null, address, target, filter);
Review comment:
and here you pass in the address name that will be part of the AddressControlImpl already.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733856808
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlTest.java
##########
@@ -580,6 +590,111 @@ public void testPurge() throws Exception {
Wait.assertEquals(0L, () -> addressControl.getMessageCount(), 2000, 100);
}
+ @Test
+ public void testReplayWithoutDate() throws Exception {
+ testReplaySimple(false);
+ }
+
+ @Test
+ public void testReplayWithDate() throws Exception {
+ testReplaySimple(true);
+ }
+
+ private void testReplaySimple(boolean useDate) throws Exception {
+ SimpleString address = RandomUtil.randomSimpleString();
+
+ AddressControl addressControl = createManagementControl(address);
+ String queue = "testQueue" + RandomUtil.randomString();
+ server.addAddressInfo(new AddressInfo(queue).addRoutingType(RoutingType.ANYCAST));
+ server.createQueue(new QueueConfiguration(queue).setRoutingType(RoutingType.ANYCAST).setAddress(queue));
+
+ ConnectionFactory factory = CFUtil.createConnectionFactory("core", "tcp://localhost:61616");
+ try (Connection connection = factory.createConnection()) {
+ Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
Review comment:
Hey @clebertsuconic
I tend to feel as if i am creating a duplication of the `session` object here.
Should i just reuse the existing `ClientSession.session` variable declared as a variable?
I am still willing to make more changes if it is not working out right.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733273289
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +574,11 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay() throws Exception {
Review comment:
Thanks @clebertsuconic
> I think you the signature should be the same one on ServerControl minus the address...
`server.replay(null, null, this.getAddress(), null, null);`
In my thinking, referencing the `this is trivial.` This is what i mean,
```
public void replay(String address, String target, String filter) throws Exception {
server.replay(null, null, this.address, target, filter);
}
```
Or i should just ignore the `this`, for now?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r731814409
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
the change required is a bit more than just this...
You have to add a method replay on ActiveMQAddressControl / ActiveMQAddressControlImpl
and delegate it to ActiveMQServerImpl (same one called by the ActiveMQServerControl)
you don't need to get the addressInfo on this case.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733775458
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
actually no..
you are supposed to be calling AddressControl
There are two components on the broker.. one is the ActiveMQServerControl... it's the master one.
then you have one for each address...
your proxy seems to be talking to ActiveMQServerControl.. and not ActiveMQAddressControl...
you are doing fine.. you're learning how things work...
Try to understand the functionality a little bit... if you like I can make a video call at some point with you and some other people.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733710363
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlTest.java
##########
@@ -607,6 +719,10 @@ protected AddressControl createManagementControl(final SimpleString address) thr
// Private -------------------------------------------------------
+ private ActiveMQServerControl createManagementControl() throws Exception {
Review comment:
this does not belong on AdressControlTest...
ActiveMQServerControlTest is the one testing the ServerControl
on AddressControlTest you should be testing the AddressControl instead.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733710721
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
you have to pass in the arguments on the proxy.invokeOperation here.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733710903
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
+ }
+
+ @Override
+ public void replay(String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
same here, you have to pass in the arguments...
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733275756
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
@clebertsuconic This is pretty clear now. Thank you so much.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733271090
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
@nbrendah the test should be something fairly simple...
Get the ActiveMQServerControlTest::testReplayWithoutDate and testReplaWithDate
copy those into ActiveMQAddressControlTest, and call the new methods that you will add:
Add 2 methods to AcctiveMQAddressControl:
```
@Operation(desc = "Makes the broker to read messages from the retention folder matching the address and filter.", impact = MBeanOperationInfo.ACTION)
void replay(
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
@Operation(desc = "Makes the broker to read messages from the retention folder matching the address and filter.", impact = MBeanOperationInfo.ACTION)
void replay(@Parameter(name = "startScanDate", desc = "Start date where we will start scanning for journals to replay. Format YYYYMMDDHHMMSS") String startScan,
@Parameter(name = "endScanDate", desc = "Finish date where we will stop scannning for journals to replay. Format YYYYMMDDHHMMSS") String endScan,
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
```
notice I only removed the address, as that will be delegated through the this.addressName
With the two copied & pasted tests from ActiveMQServerControl into ActiveMQAddressControlTest (select a random address for the test), you should be able to finish the task.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733271478
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
Notice, I'm suggesting you add two methods into ActiveMQAddressControl.. one with the date, one without the date, just list the ActiveMQServerControl.. but those are just there as overloads.
let me know what you think and how you progress on 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic edited a comment on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948241083
also, next time you mvn build, use the -Pdev profile at least once.. it takes some extra time, but it will run the check style.. etc...
the -Derrorprone may also help with the PR checks to make sure it passes it.
```
mvn -Pdev install -Derrorprone
```
One thing I do all the time to not run tests:
```
mvn -Pdev install -DskipTests=true -Derrorprone
```
I actually have an alias on my OS to those scripts...
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733743150
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/AddressControlUsingCoreTest.java
##########
@@ -178,6 +178,16 @@ public long purge() throws Exception {
return (long) proxy.invokeOperation("purge");
}
+ @Override
+ public void replay(String startScan, String endScan, String target, String filter) throws Exception {
+ proxy.invokeOperation("replay");
Review comment:
Hey @clebertsuconic
I guess the method `proxy.invokeOperation` calls AddressControl.replay(.....) so there is no need of passing the `this.getAddress()` as an argument
ie
```
@Override
public void replay(String target, String filter) throws Exception {
proxy.invokeOperation("replay", this.getAddress(), target, filter);
}
```
Am sorry, it seams am taking too much time on the would be simplest issues
I am just not familiar with this code base. Am sure with some few days, my speed will boost.
Thanks once again
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r733259828
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/AddressControlImpl.java
##########
@@ -574,6 +574,11 @@ public long purge() throws Exception {
return totalMsgs;
}
+ @Override
+ public void replay() throws Exception {
Review comment:
I think you the signature should be the same one on ServerControl minus the address...
other than that you're spot on on the 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#issuecomment-948801364
the test is not runnable as is.. (did you try running these test?)
but I'm impressed that with a little tweak I made to the test (Like I needed to configure the retention folder, and some other small tweak, the tests are passing...)
I believe this task will be complete now ... give me 5 min.. and I will let you know
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] nbrendah commented on a change in pull request #3801: ARTEMIS-3523: Expose replay through AddressControl
Posted by GitBox <gi...@apache.org>.
nbrendah commented on a change in pull request #3801:
URL: https://github.com/apache/activemq-artemis/pull/3801#discussion_r731498792
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -4443,7 +4443,7 @@ public void reloadConfigurationFile() throws Exception {
@Override
public void replay(String address, String target, String filter) throws Exception {
- server.replay(null, null, address, target, filter);
+ server.replay(null, null, this.getAddressInfo(address), target, filter);
Review comment:
I think the requirement was `this.address` but the `this` property does not have access to the address field. Unless there is some class we have to implement or extend.
Or this is implemented in a wrong position.
Looking forward to seeing your suggestions @clebertsuconic
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org