[
https://issues.apache.org/jira/browse/QPID-6670?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14660096#comment-14660096
]
Lorenz Quack commented on QPID-6670:
------------------------------------
Here are my review comments (ordered by file):
* The changes to TaskExecuterImpl, TopicExchange, and AMQChannel seem
unrelated to this JIRA
* AbstractVirtualHost
** {{removeExchangeAsync()}} does not use the {{force}} parameter and neither
does any other implementation
* ExchangeImpl
** declaration of {{deleteWithChecks()}} seems unnecessary since it is already
declared on {{Exchange}}
* BindingImpl
** I am a bit puzzled by the if statement in {{doDelete()}}. *If* the method
can be called multiple times the calls should surely return the same future,
right?
* AbstractExchange
** Is there any value in keeping the TODO in {{removeBindingAsync()}}?
** {{deleteWithChecks()}}
*** {{removeBindingFuture}} can be created with the correct size
({{_bindings.size()}})
*** Is the {{atLeastOne}} future really necessary? I have not checked this but
would expect {{Futures.allAsList()}} to return the equivalent of an
{{Futures.immediateFuture(Collections.emptyList())}} if called with an empty
list.
* AbstractQueue
** {{deleteAndReturnCount()}}
*** same as above: do we need the {{atLeastOne}} future and if the TODO still
of value?
*** the {{removeFuture}} in the for-loop seems a bit verbose. I would inline
it.
*** Not sure about this one but the use of {{returnCountFuture}} and the
anonymous {{ListenableFuture}} in the return statement seem wired to me. I
believe this can be implemented more elegant with {{Futures.transform()}}.
Something along these lines: {code:java}final int queueDepth =
getQueueDepthMessages();
/* ... */
return Futures.transform(result, new Function(){
@Override
public Object apply(final Object input)
{
return queueDepth;
}
});
{code}
*** I noticed that {{closeAsync}} isn't being called anymore? Is this on
purpose or by mistake?
* The rest of the patch and the general logic seem good to me.
> Exchange deletion does not await binding deletion leading to possibility of a
> IllegalStateException : Task executor is not in ACTIVE state
> ------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-6670
> URL: https://issues.apache.org/jira/browse/QPID-6670
> Project: Qpid
> Issue Type: Bug
> Components: Java Broker
> Affects Versions: qpid-java-6.0
> Reporter: Keith Wall
> Assignee: Lorenz Quack
> Fix For: qpid-java-6.0
>
>
> As highlighted by
> QueueDeclareTest.testDeclareIgnoresDurableFlagIfNonDurableQueueAlreadyExists
> failing on Jenkins, there is a race between the bindings deletion (part of
> which can happen asynchronously) and the shutdown of the virtualhost's task
> executor.
> In the unlucky case, the asynchronous task is submitted after the task
> executor has shutdown.
> This could manifest when stopping a VH (either by way of a Management
> operation, Broker shutdown, or HA mastership change)
> {noformat}
> java.lang.IllegalStateException: Task executor is not in ACTIVE state
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.checkState(TaskExecutorImpl.java:310)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submit(TaskExecutorImpl.java:141)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.doOnConfigThread(AbstractConfiguredObject.java:499)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.setDesiredState(AbstractConfiguredObject.java:1315)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.deleteAsync(AbstractConfiguredObject.java:1837)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.delete(AbstractConfiguredObject.java:1766)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.exchange.AbstractExchange.removeBinding(AbstractExchange.java:666)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:na]
> at
> org.apache.qpid.server.exchange.AbstractExchange.access$000(AbstractExchange.java:80)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:na]
> at
> org.apache.qpid.server.exchange.AbstractExchange$1.stateChanged(AbstractExchange.java:138)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:na]
> at
> org.apache.qpid.server.exchange.AbstractExchange$1.stateChanged(AbstractExchange.java:132)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:na]
> at
> org.apache.qpid.server.binding.BindingImpl.doDelete(BindingImpl.java:208)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at sun.reflect.GeneratedMethodAccessor23.invoke(Unknown Source)
> ~[na:na]
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> ~[na:1.7.0_67]
> at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_67]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1194)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainStateIfOpenedOrReopenFailed(AbstractConfiguredObject.java:1162)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.access$1500(AbstractConfiguredObject.java:83)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$15.call(AbstractConfiguredObject.java:1354)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$15.call(AbstractConfiguredObject.java:1316)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:507)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:500)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.executeTask(TaskExecutorImpl.java:317)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.access$700(TaskExecutorImpl.java:48)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper$1.run(TaskExecutorImpl.java:361)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at java.security.AccessController.doPrivileged(Native Method)
> ~[na:1.7.0_67]
> at javax.security.auth.Subject.doAs(Subject.java:356) ~[na:1.7.0_67]
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper.call(TaskExecutorImpl.java:356)
> ~[qpid-broker-core-6.0.0-SNAPSHOT.jar:6.0.0-SNAPSHOT]
> at java.util.concurrent.FutureTask.run(FutureTask.java:262)
> ~[na:1.7.0_67]
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
> ~[na:1.7.0_67]
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
> ~[na:1.7.0_67]
> at java.lang.Thread.run(Thread.java:745) ~[na:1.7.0_67]{noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
