You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by dudaerich <gi...@git.apache.org> on 2017/08/23 07:12:07 UTC
[GitHub] activemq-artemis pull request #1486: ARTEMIS-1368 Artemis gets to state when...
GitHub user dudaerich opened a pull request:
https://github.com/apache/activemq-artemis/pull/1486
ARTEMIS-1368 Artemis gets to state when it doesn't respond to producer
There is a leak on replication tokens in the moment when a backup is
shutdowned or killed and the ReplicationManager is stopped. If there
are some tasks (holding replication tokens) in the executor, these
tokens are simply ignored and replicationDone method isn't called on
them. Because of this, some tasks in OperationContextImpl cannot be
finished.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dudaerich/activemq-artemis ARTEMIS-1368
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1486.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1486
----
commit 42322f8e8cb2bd497d852bd1bd7e6ae77fbbc8b1
Author: Erich Duda <ed...@redhat.com>
Date: 2017-08-22T19:48:19Z
ARTEMIS-1368 Artemis gets to state when it doesn't respond to producer
There is a leak on replication tokens in the moment when a backup is
shutdowned or killed and the ReplicationManager is stopped. If there
are some tasks (holding replication tokens) in the executor, these
tokens are simply ignored and replicationDone method isn't called on
them. Because of this, some tasks in OperationContextImpl cannot be
finished.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@dudaerich agreed, essentially you're catching the case where enabled is changed between time the execute is called and its actually run, i think there maybe some possible tidy up could be done.
currently:
```
if (enabled) {
replicationStream.execute(() -> {
if (enabled) {
pendingTokens.add(repliToken);
flowControl(packet.expectedEncodeSize());
replicatingChannel.send(packet);
} else {
packet.release();
repliToken.replicationDone();
}
});
} else {
// Already replicating channel failed, so just play the action now
runItNow = true;
packet.release();
}
// Execute outside lock
if (runItNow) {
repliToken.replicationDone();
}
```
as it seems the old lock that once existed is remove run it now can probably be removed and look like the same logic block inside you added to execute:
e.g.
```
if (enabled) {
replicationStream.execute(() -> {
if (enabled) {
pendingTokens.add(repliToken);
flowControl(packet.expectedEncodeSize());
replicatingChannel.send(packet);
} else {
packet.release();
repliToken.replicationDone();
}
});
} else {
// Already replicating channel failed, so just play the action now
packet.release();
repliToken.replicationDone();
}
```
next is the question of do we need to have this duplicated check, could this just be all inside the runnable, do we need to do double check? e.g just have
```
replicationStream.execute(() -> {
if (enabled) {
pendingTokens.add(repliToken);
flowControl(packet.expectedEncodeSize());
replicatingChannel.send(packet);
} else {
packet.release();
repliToken.replicationDone();
}
});
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@michaelandrepearce there is an intermittent failure on the testsuite that my be caused by this.
@dudaerich nice catch indeed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@dudaerich that is what i would do. Without an integration test around this logic, I would suggest you run the manual test case as well in wildfly to ensure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by dudaerich <gi...@git.apache.org>.
Github user dudaerich commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
Thanks guys. I've updated the commit based on your feedback. I am running the wildfly test now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@mtaylor @clebertsuconic https://builds.apache.org/job/ActiveMQ-Artemis-PR-Build/3697/#showFailuresLink I've noticed this on a few builds, so assuming its nothing related to this code change, but are you guys aware?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@dudaerich Thanks Erich. This looks good. I will merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@michaelandrepearce I have noticed those errors before. I suspected environment issues in the Apache CI infra. But we should try and get them resolved. Having intermittent failures makes it harder to spot new ones. Thanks for the nudge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@clebertsuconic thats good news on test case front. thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
Nice catch on this @dudaerich. I think @michaelandrepearce suggestions make sense. The code needs a little refactor. Remove the double check. There is actually another leak here. First line in the sendReplicatePacket()
```java
if (!enabled) return null.
```
Can you also fix this. Cheers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@dudaerich is there a test case to prove the fix and more importantly ensure this doesn't get regressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by dudaerich <gi...@git.apache.org>.
Github user dudaerich commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@michaelandrepearce I agree with all you wrote in the previous comment. I think the last code snippet is correct.
So what steps do you suggest to do? Should I update the commit to have "if enabled" check only inside of lambda and then run the tests again?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by dudaerich <gi...@git.apache.org>.
Github user dudaerich commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
@michaelandrepearce the issue was hit in Wildfly when the Artemis was under the load and its backup was turned off. I did heavy testing using the Wildfly tests and it seems that the issue is fixed by this change. I also run the Artemis test suite for regression testing - it looks OK. See https://issues.jboss.org/browse/JBEAP-12681 for more info.
@clebertsuconic reviewed the fix and confirm that I can open this PR. Basically this is just improvement of his previous fix 387fca584e7cb66656f23b29a24169bb2fb8fc67
It would be nice to have a test for this but I am not sure how to write it properly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #1486: ARTEMIS-1368 Artemis gets to state when it doe...
Posted by dudaerich <gi...@git.apache.org>.
Github user dudaerich commented on the issue:
https://github.com/apache/activemq-artemis/pull/1486
Thanks guys :)
The wildfly test passed. I ran it 5 times. Without the fix the test failed in the first or second run.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #1486: ARTEMIS-1368 Artemis gets to state when...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1486
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---