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/05/05 16:10:09 UTC
[GitHub] [activemq-artemis] ehsavoie opened a new pull request #3117: NO-JIRA Wrong key used for removal.
ehsavoie opened a new pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117
The key is of type String and not Queue so this wouldn't work properly
----------------------------------------------------------------
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] cshannon commented on pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#issuecomment-626666785
@clebertsuconic - As suspected there is actually more than one mistake here. I'm working on a patch now and an update to the tests to verify things are good. I should be done later today and I will close out this PR and open up a new one against ARTEMIS-2752.
----------------------------------------------------------------
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 #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420234721
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
The best would be to have a JIRA, and a test exposing it.
if you can't do it, just open the JIRA, and reference this as the patch, with some instructions on how to hit the issue.
----------------------------------------------------------------
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] cshannon commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420673426
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
Thanks for adding me @michaelandrepearce. Yes this is a feature I added and have been testing out a bit so I want to make sure it's working properly.
After taking a quick 5 min look this seems that it could be a mistake since a SimpleString should be stored, however I don't think this is the right fix either. I believe the queue name is stored and not the address.
I am not back into the office until Monday but when I get back I will take a closer look at this to verify it is a mistake. Assuming it is wrong I will create a patch and some new tests to show it has been fixed.
On another note, automated tools like error prone are great to help find errors. However, Clebert is right about needing a Jira and tests. Tests still need to be written to demonstrate the issue is fixed because I think in this case the fix is actually wrong as well and a test would show 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] ehsavoie commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420726362
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
I've created ARTEMIS-2752 to track this
@cshannon you are correct you are using queue.getName() as line 220 shows (https://github.com/apache/activemq-artemis/pull/3117/files#diff-29849e7d9fe61b8003e942e5d94b6b88R220)
----------------------------------------------------------------
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 #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r421551694
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
@cshannon I will do a release next week (about to send the HEADS up)
do you think you could fix this by then?
----------------------------------------------------------------
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] cshannon commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r421571786
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
Probably, I should have time to work on it Monday when I'm back.
----------------------------------------------------------------
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] cshannon commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420736990
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
@ehsavoie - Thanks, I assigned it to myself.
----------------------------------------------------------------
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] ehsavoie commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
ehsavoie commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420624596
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
I don't know how to hit the issue. This was detected by error-prone
----------------------------------------------------------------
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 a change in pull request #3117: NO-JIRA Wrong key used for removal.
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420628001
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
//check if the queue has been tracked by this divert and if so remove the consumer
- if (entry.getValue().remove(queue)) {
+ if (entry.getValue().remove(queue.getAddress())) {
Review comment:
Can we get @cshannon to review, this was a feature / addition he added
----------------------------------------------------------------
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