You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by clebertsuconic <gi...@git.apache.org> on 2017/10/18 12:19:09 UTC
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/1596
ARTEMIS-450 Deadlocked queues over AMQP after too many cancellations
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis amqp-deadlock
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1596.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 #1596
----
commit cdcb7fc0a9408e8fbf506da7d9d64a1e52541267
Author: Clebert Suconic <cl...@apache.org>
Date: 2017-10-17T19:54:49Z
ARTEMIS-450 Fixing deadlock over lots of rollbacks and Queue.addHead
commit 1a8887a9c40477663ece7d4c87af72bd5f3fdefb
Author: Clebert Suconic <cl...@apache.org>
Date: 2017-10-18T02:30:59Z
ARTEMIS-1462 Fixing QueueControlTest
----
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@clebertsuconic It is not following the [principle of least astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment) from the API (and doc) perspective: if you remove the `getInitialDelay`probably you could use safetly that change, but considering that all the other properties are exposed, probably it is not the right choice.
That's my 2 cents on it: I'm happy anyway that the functionality isn't broken (for my usage I mean) :+1:
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
using your own words.. the least astonishment way would be to have the initialDelay nullable (or -1 on this case).... I'm fixing what was broken before.
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145425664
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
@clebertsuconic When I have a very big period but I need the first call to happen right after the start of the component. IMO a user expectation is that the component start will honour the configuration provided if the componet was in a stopped state...
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@franz1981 It's just that I changed the meaning of getInitialDelay... -1 meaning unset.. and use the previous semantic... the change you did broke some usage.. so I had to have a null state for initialDelay where I would get the previous semantic... that's all
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145411963
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
I've built up this test:
```
@Test
public void testVerifyInitialDelayChanged() {
final long initialDelay = 10;
final long period = 100;
final ActiveMQScheduledComponent local = new ActiveMQScheduledComponent(scheduledExecutorService, executorService, initialDelay, period, TimeUnit.MILLISECONDS, false) {
@Override
public void run() {
}
};
local.start();
final long newInitialDelay = 1000;
//the parameters are valid?
assert initialDelay != newInitialDelay && newInitialDelay != period;
local.setInitialDelay(newInitialDelay);
local.stop();
Assert.assertEquals("the initial dalay can't change", newInitialDelay, local.getInitialDelay());
}
```
And it fail: maybe is better to create a `start(long forceInitialDelay)` method and call it into the restart, wdyt?
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145417351
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
@clebertsuconic it is "initial" on each start after a stop (eg how i use it of JDBC)
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145455314
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -90,7 +90,7 @@ public ActiveMQScheduledComponent(ScheduledExecutorService scheduledExecutorServ
long checkPeriod,
TimeUnit timeUnit,
boolean onDemand) {
- this(scheduledExecutorService, executor, checkPeriod, checkPeriod, timeUnit, onDemand);
+ this(scheduledExecutorService, executor, -1, checkPeriod, timeUnit, onDemand);
--- End diff --
I'm receiving an error in the test `testVerifyDefaultInitialDelay`:
```
java.lang.AssertionError: The initial delay must be defaulted to the period
Expected :100
Actual :-1
```
Modifying things like this (and leaving the constructor as it is) doesn't break any tests:
```
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
stop();
//do not need to start with the initialDelay: the component was already running
start(this.period);
}
}
private void start(final long initialDelay) {
if (future != null) {
// already started
return;
}
if (scheduledExecutorService == null) {
scheduledExecutorService = new ScheduledThreadPoolExecutor(1, getThreadFactory());
startedOwnScheduler = true;
}
if (onDemand) {
return;
}
this.millisecondsPeriod = timeUnit.convert(period, TimeUnit.MILLISECONDS);
if (period >= 0) {
future = scheduledExecutorService.scheduleWithFixedDelay(runForScheduler, initialDelay, period, timeUnit);
} else {
logger.tracef("did not start scheduled executor on %s because period was configured as %d", this, period);
}
}
@Override
public synchronized void start() {
start(this.initialDelay);
}
```
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
your test wasn’t validating semantics. The change is good.
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145416958
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
@clebertsuconic But if (at the end of the test) I will start the component, it will have a changed configuration...it can lead to subtle bugs...
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@franz1981 lets not waste time.. this is such a small component... you added a new parameter that was clashing with previous usage.. all I'm doing is fix it accordingly to the previous version after the tests you broke.
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@franz1981 I'm using an initialDelay = -1 by default now, and added some code around on start.
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@clebertsuconic It is a get of field after a set of the same that is not getting the configured/expected value: it is not what a user could expect.
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1596
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145424777
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
What do you need an initialDelay different from the frequency anyways? What is the usecase?
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@franz1981 nope.. you broke a test Franz.
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145416018
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
Nope. Initial delay should be at the beginnnjng only. Why would you want a further initial delay when it’s not initial any longer. Just to keep it simple.
---
[GitHub] activemq-artemis issue #1596: ARTEMIS-450 Deadlocked queues over AMQP after ...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1596
@clebertsuconic I do not agree with that: the change I did was validated with tests and the documentation says that when the initial delay isn't specified is defaulted to period, so the expectation on it is that it will be defaulted to period in a `getInitialDelay` too.
As I said, for me it's the same but IMO if you want to add a new semantic over a null initialDelay you need to specify it in some way (ie docs/tests and/or even better, on the API) in order to avoid subtle bugs and/or to avoid to change tests that are only checking a get after a set.
---
[GitHub] activemq-artemis pull request #1596: ARTEMIS-450 Deadlocked queues over AMQP...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1596#discussion_r145408498
--- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java ---
@@ -251,6 +251,9 @@ public synchronized boolean isStarted() {
// this will restart the scheduled component upon changes
private void restartIfNeeded() {
if (isStarted()) {
+ // it has already been through an initial delay,
+ // now we just use the next interval
+ this.initialDelay = period;
--- End diff --
nice!!!!! :+1:
---