[
https://issues.apache.org/jira/browse/QPID-8066?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16340908#comment-16340908
]
Alex Rudyy commented on QPID-8066:
----------------------------------
Keith,
I reviewed the changes in the attached patch.
In general I believe that proposed changes is a move in right direction. It
looks like the changes prepare the ground for QPID-7197. Also, they address
partially QPID-7547 which might be closed after committing the patch.
Here are my review comments
* BDBHAVirtualHostNodeImpl
** {{_helpers} field itself and setting of {{_helper}} field in
{{beforeDelete}} does not look necessary to me. It seems that local variable in
{{onDelete}} can be introduced for this purpose.
** Flag {{_isClosed}} is set on both deletion and close. Would it be better to
rename the flag into {{_isClosingOrDeleting}} to reflect that it can be set
from both operations?
** {{#removeRemoteReplicationNode}} invokes {{deleteAsync}}. I think that
{{deleteNoChecks}} should be called instead off {{deleteAsync}} in order to
avoid unnecessary authorization checks.
** In {{onDelete}} of BDB HA VHN its VH is get closed. We discussed that before
and you implemented exactly what we discussed. Though, after reviewing the
changes, it looks more logical to me to close all direct children of CO with
{{managesChildStorage=true}} in {{AbstractConfiguredObject#deleteChildren}}.
Thus, no call to close the children would be required in the implementation of
{{onDelete}}.
* AbstractExchange
** {{#onDelete}}; why performDelete is called only on condition {{getState() !=
State.UNINITIALIZED}}? It seems to me that this condition need to be removed
otherwise a reference to _alternateBindingDestination will not removed and
binding count statistics on queues will not be updated.
* AmqpPortImpl
** {{_closing}}; is set on deletion and close. Would it be better to rename the
flag into {{_isClosingOrDeleting}} to reflect that it can be set from both
operations?
* AbstractConfiguredObject
** In {{deleteChildren}} when {{managesChildStorage=true}}, the immediate
future is returned. Would it be better to close the children and return close
future? Though, it is not required now, but taking that it is a generic code,
potentially, on close some system resources can be cleaned up. At the moment,
COs managing children close the children by themselves in {{doDelete}} which
looks to me like a duplication of code.
** {{CreateExceptionHandler#handleException}} invokes {{deleteAsync}}. IMHO, it
should call {{deleteNoChecks}} in order to avoid making unnecessary
authorization checks.
** when child is not instance AbstractConfiguredObject or
AbstractConfiguredObjectProxy, it seems it will not be deleted. It does not
look like a problem at the moment, but, it points to the potential gap in a
public API of {{ConfiguredObject}} interface. We might need a public method to
perform the delete checks.
* AbstractQueue
** {{_deleteQueueDepthFuture}} is set twice: once prematurely at the beigining
of {{#performDelete}} and second time at the end of {{#performDelete}}
** Method {{deleteAndReturnCountAsync}} calls {{deleteAsync}}. It seems it
should call {{deleteNoCheck}} as authorization and checks are done in
{{deleteAndReturnCountAsync}}.
* QueueConsumerImpl
** {{deleteAsync}} is invoked from {{onClose}}. As result authorization check
would be performed which is not really necessary. Would it be better to call
{{deleteNoChecks}} instead of {{deleteAsync}}
* AbstractAuthenticationManager
** Method {{performDelete}} can be inlined.
* Immediate future is created explicitly in {{doDelete}}. Would it be better to
call {{super.doDelete()}} for consistency reasons?
* AbstractKeyStore
** Method {{deleteIfNotInUse}} is not used and can be safely removed.
** Method {{doDelete}} can call and return {{super.doDelete}} instead of
creation of intermediate future
* AbstractTrustStore
** Immediate future is created explicitly in {{doDelete}}. Would it be better
to call {{super.doDelete()}} for consistency?
* AbstractAMQPSession
** Immediate future is created explicitly in {{doDelete}}. Would it be better
to call {{super.doDelete()}} for consistency?
** Task {{_deleteModelTask}} calls {{deleteAsync}}. IMHO, it should call
{{deleteNoChecks}} to avoid unnecessary authorization and checks.
* PrincipalDatabaseAuthenticationManager
** In method {{addChildAsync}} on {{RuntimeException}} the user is deleted
asynchronously by calling {{principalAdapter.deleteAsync();}}. IMHO,
{{deleteNoChecks}} should be called instead to avoid no necessary auth checks.
> [Broker-J] Virtual host logger rules are left over in configuration store
> after deletion of virtual host logger on provided virtual host causing
> virtualhost restart failure
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-8066
> URL: https://issues.apache.org/jira/browse/QPID-8066
> Project: Qpid
> Issue Type: Bug
> Components: Broker-J
> Affects Versions: qpid-java-6.0, qpid-java-6.0.1, qpid-java-6.0.2,
> qpid-java-6.0.3, qpid-java-6.0.4, qpid-java-6.0.5, qpid-java-6.1,
> qpid-java-6.0.6, qpid-java-6.1.1, qpid-java-6.1.2, qpid-java-6.0.7,
> qpid-java-6.1.3, qpid-java-6.0.8, qpid-java-6.1.4, qpid-java-broker-7.0.0,
> qpid-java-6.1.5
> Reporter: Alex Rudyy
> Assignee: Keith Wall
> Priority: Critical
> Fix For: qpid-java-broker-7.0.1, qpid-java-broker-7.1.0
>
> Attachments:
> 0001-QPID-8066-Broker-J-Remove-all-children-records-recur.patch,
> QPID-8066.tar.bz2
>
>
> Virtual host logger rules are left over in configuration store after deletion
> of virtual host logger on provided virtual host. On restart of VHN or Broker,
> virtual host recovery fails with IllegalArgumentException as the one below:
> {noformat}
> java.lang.IllegalArgumentException: Recovered configured object record
> BDBConfiguredObjectRecord [id=8e9c5547-c41b-4443-9333-48dac61f3b40,
> type=VirtualHostLogInclusionRule, name=test, parents={}] has no recorded
> parents and is not a valid child type [[interface
> org.apache.qpid.server.model.VirtualHost, interface
> org.apache.qpid.server.model.RemoteReplicationNode]] for the root
> BDBVirtualHostNodeImplWithAccessChecking
> [id=591aa6d9-c2e0-474c-a0a9-86eb14bc3c6a, name=bdb,
> storePath=/home/alex/.qpid-7.1.0-SNAPSHOT/bdb/config]
> at
> org.apache.qpid.server.store.GenericRecoverer.resolveDiscontinuity(GenericRecoverer.java:119)
> at
> org.apache.qpid.server.store.GenericRecoverer.performRecover(GenericRecoverer.java:90)
> at
> org.apache.qpid.server.store.GenericRecoverer.access$000(GenericRecoverer.java:41)
> at
> org.apache.qpid.server.store.GenericRecoverer$1.execute(GenericRecoverer.java:59)
> at
> org.apache.qpid.server.store.GenericRecoverer$1.execute(GenericRecoverer.java:55)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submitWrappedTask(TaskExecutorImpl.java:165)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.run(TaskExecutorImpl.java:190)
> at
> org.apache.qpid.server.store.GenericRecoverer.recover(GenericRecoverer.java:54)
> at
> org.apache.qpid.server.store.VirtualHostStoreUpgraderAndRecoverer.recover(VirtualHostStoreUpgraderAndRecoverer.java:1085)
> at
> org.apache.qpid.server.store.VirtualHostStoreUpgraderAndRecoverer.upgradeAndRecover(VirtualHostStoreUpgraderAndRecoverer.java:1058)
> at
> org.apache.qpid.server.virtualhostnode.AbstractStandardVirtualHostNode.activate(AbstractStandardVirtualHostNode.java:91)
> at
> org.apache.qpid.server.virtualhostnode.AbstractVirtualHostNode.doActivate(AbstractVirtualHostNode.java:162)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1524)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1503)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1070)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1064)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2639)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2635)
> at java.security.AccessController.doPrivileged(Native Method)
> at javax.security.auth.Subject.doAs(Subject.java:360)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22.onSuccess(AbstractConfiguredObject.java:2634)
> at
> com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1237)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$ImmediateIfSameThreadExecutor.execute(TaskExecutorImpl.java:400)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.execute(TaskExecutorImpl.java:183)
> at
> com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:911)
> at
> com.google.common.util.concurrent.AbstractFuture.addListener(AbstractFuture.java:645)
> at
> com.google.common.util.concurrent.AbstractFuture$TrustedFuture.addListener(AbstractFuture.java:101)
> at
> com.google.common.util.concurrent.Futures.addCallback(Futures.java:1209)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.addFutureCallback(AbstractConfiguredObject.java:2629)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1063)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$7.performAction(AbstractConfiguredObject.java:1048)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$7.performAction(AbstractConfiguredObject.java:1038)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.applyToChildren(AbstractConfiguredObject.java:1311)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1037)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:589)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:576)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:637)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:630)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submitWrappedTask(TaskExecutorImpl.java:165)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submit(TaskExecutorImpl.java:153)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.doOnConfigThread(AbstractConfiguredObject.java:629)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.openAsync(AbstractConfiguredObject.java:575)
> at
> org.apache.qpid.server.model.AbstractSystemConfig.makeActive(AbstractSystemConfig.java:304)
> at
> org.apache.qpid.server.model.AbstractSystemConfig.activate(AbstractSystemConfig.java:280)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1524)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1503)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1070)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1064)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2639)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2635)
> at java.security.AccessController.doPrivileged(Native Method)
> at javax.security.auth.Subject.doAs(Subject.java:360)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$22.onSuccess(AbstractConfiguredObject.java:2634)
> at
> com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1237)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$ImmediateIfSameThreadExecutor.execute(TaskExecutorImpl.java:400)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl.execute(TaskExecutorImpl.java:183)
> at
> com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:911)
> at
> com.google.common.util.concurrent.AbstractFuture.addListener(AbstractFuture.java:645)
> at
> com.google.common.util.concurrent.AbstractFuture$TrustedFuture.addListener(AbstractFuture.java:101)
> at
> com.google.common.util.concurrent.Futures.addCallback(Futures.java:1209)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.addFutureCallback(AbstractConfiguredObject.java:2629)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1063)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:589)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:576)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:637)
> at
> org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:630)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper$1.run(TaskExecutorImpl.java:320)
> at java.security.AccessController.doPrivileged(Native Method)
> at javax.security.auth.Subject.doAs(Subject.java:360)
> at
> org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper.call(TaskExecutorImpl.java:313)
> at
> com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:111)
> at
> com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:58)
> at
> com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:75)
> at
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
> at
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
> at
> org.apache.qpid.server.bytebuffer.QpidByteBufferFactory.lambda$null$0(QpidByteBufferFactory.java:464)
> at java.lang.Thread.run(Thread.java:745)
> {noformat}
> The rule entry can only be deleted from outside of the Broker which would be
> difficult to achieve with BDB store. If entry cannot be deleted, the Virtual
> host node removal can only resolve the issue which means the loss of the
> configuration and messages. Though, configuration and messages can be
> exported from impacted VHN and re-imported into re-created VHN.
> Alternatively, all VH logging rules need to be deleted before the deletion of
> VH logger
> I would vote for fixing this issue in 7.0.1 and porting the changes into 6.x
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
