[
https://issues.apache.org/jira/browse/QPID-7622?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15940616#comment-15940616
]
Alex Rudyy commented on QPID-7622:
----------------------------------
Keith,
I reviewed commits made as part of this JIRA and here are my review comments:
* Broker code comments
** I think it make sense to remove {{org.apache.qpid.messaging.util.JAddr}},
some of the classes {{JAddr}} depends on like {{PyPrint}}, and bash script
{{qpid-jaddr}}. Nobody really using this and it looks to me that it was added
for testing purposes to test ADDRESS syntax parsing.
** Redundant import of {{org.apache.qpid.server.bytebuffer.ByteBufferRef}} in
{{NonPooledByteBufferRef}}
** Redundant import of {{org.apache.qpid.server.bytebuffer.QpidByteBuffer}} in
{{QpidByteBufferOutputStream}}
** It looks like {{org.apache.qpid.server.common.AMQPFilterTypes}} belongs to
package {{org.apache.qpid.server.filter}}
** Unsused constant
{{org.apache.qpid.server.common.ServerPropertyNames#QPID_TEMPORARY_QUEUE_PREFIX}}.
The constant can be removed. The remaining constants in
{{ServerPropertyNames}} can be moved into {{CommonProperties}} and
{{ServerPropertyNames}} class can be removed. Constants from
{{ConnectionStartProperties}} can be moved into {{CommonProperties}} as well.
If {{ServerPropertyNames}} and {{ConnectionStartProperties}} are merged into
{{CommonProperties}} packages {{org.apache.qpid.server.common}} and
{{org.apache.qpid.server.properties}} can be removed. Potentially
{{CommonProperties}} can be renamed into {{BrokerPropertyNames}}.
** Redundant import of {{import java.util.Iterator}} in {{ServerInputHandler}}
** {{ServerConnection}}
*** {{org.apache.qpid.server.protocol.v0_10.ServerConnection#resume}} is not
used.
*** {{org.apache.qpid.server.protocol.v0_10.ServerConnection#getLocalAddress}}
is not used.
*** {{org.apache.qpid.server.protocol.v0_10.ServerConnection#_port}} field is
redundant. Port can be taken from {{AMQPConnection_0_10#getPort()}}.
***
{{org.apache.qpid.server.protocol.v0_10.ServerConnection#getRemoteContainerName}}
can be deleted as it is only called from
{{AMQPConnection_0_10Impl#getRemoteContainerName}} and
{{AMQPConnection_0_10Impl}} already has it set as clientId. Thus
{{this.getClinetId()}} can be called instead.
*** Redundunt casts to {{ServerConnectionDelegate}} and {{ServerSession}} in
multiple places.
*** Synchronization on {{#lock}} looks redundant to me. Should we remove it and
make member variables volatile and replace such multi-thread unsafe classes
like {{HashMap}} with {{ConcurrentHashMap}}?
*** Intrinsic lock synchronization in method {{#registerSession}} also looks
reduntant.
*** Intrinsic lock synchronization in methods {{#block}} and {{#unblock}} look
unnecessary, as a caller {{AbstractVirtualHost}} synchronizes on its own
{{#_connections}} field in methods {{AbstractVirtualHost#block}} and
{{AbstractVirtualHost#unblock}} but we might need to modify
{{AbstractVirtualHost#registerConnectionAsync}} to synchronize on
{{#_connections}} as well when connection is registered.
** {{ServerConnectionDelegate}}
*** org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#writerIdle
is not used
***
org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#getClientProperties
is not used.
***
org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#getClientVersion
is not used.
***
org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#getClientProduct
is not used.
***
org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#getRemoteProcessPid
is not used.
*** Invocation of
org.apache.qpid.server.protocol.v0_10.ServerConnectionDelegate#getClientId can
be deleted. Only ServerConnection calls it in
ServerConnection#getRemoteContainerName() but the latter is not required. See
comments above.
*** A lot of redundant casts to ServerConnection.
*** @Override annotation is missed on methods #control, #command, #error,
#handle, connectionOpen.
*** It seems ServerConnectionDelegate does not bring much but makes code more
convoluted. Would it make sense to merge ServerConnectionDelegate into
ServerConnection? If not, i would move implementations of methods
connectionOpen, connectionClose, etc into ServerConnection and make delegate
simply call corresponding ServerConnection methods. IMHO, that would make code
more cleaner. The cyclic dependencies between these 2 classes look weird.
** {{AMQPConnection_0_10Impl}}
***
org.apache.qpid.server.protocol.v0_10.AMQPConnection_0_10Impl#getRemoteContainerName
can call internally getClientId instead of calling
ServerConnection#getRemoteContainerName() as both return the same value as they
set from "clientName" connectionStart property. Method
ServerConnection#getRemoteContainerName() can be deleted.
** {{SessionListenr}}
***
{{org.apache.qpid.server.protocol.v0_10.SessionListener}}/{{ServerSession$DefaultSessionListener}}
look redundant to me and can be safely deleted.
*** Invocation of {{ServerSession#getSessionListener()}} in
{{ServerSessionDelegate}} can be replaced with {{ServerSession#exception()}}
method if required, but taking that currently exception only is logged, there
is nothing stopping us from logging it SSD.
** {{ServerSession}}
*** Do we really need synchronization on {{#processedLock}}, {{#commandsLock}},
{{#results}}? IMHO, we need to remove those synhronized blocks but declare
member variables volatile and replace such multi-thread unsafe types like
{{ArrayList}} with their thread-safe equivalents.
*** Synchronization on {{#_blockingEntities}} seems redundant as well.
*** Method {{#addCredit}} is unused
*** Method {{#drainCredit}} is unused
*** Member {{#credit}} is only used in {{#addCredit }}and {{#drainCredit}} and
can be removed together with the methods.
*** Constructor parameter "expire" is unused and can be removed
*** Method {{#getCommandsOut}} is unused
*** Method {{#processed(Range range)}} is unused
*** Method {{#isBytesFull}} is unused
*** Members {{#byteLimit}} and {{#commandBytes}} can be removed as well on
removal of method {{#isBytesFull}}
*** Member {{#_failoverRequired}} needs to be removed together with not needed
method {{#checkFailoverRequired}}
*** Method {{#messageTransfer(String, MessageAcceptMode, MessageAcquireMode,
Header, String, Option...)}} is unused
*** Method{{ #getChannelId}} is unused
*** There is a number of redundant casts to {{ServerConnection}}
** Remaining 0.8 protocol classes in broker-core (package
{{org.apache.qpid.server.protocol.v0_8}})
I believe we should move them into "0.8 protocol" module and add into bdbstore
module a runtime optional dependency to "0.8 protocol" module. Only upgrader
from v5 to v6 requires 0.8 classes to *** perform the upgrade. If the store
version is v6 or above, the absence of "0.8 protocol" module in classpath
should not break anything as upgraders are loaded using reflection. Users
upgrading from v5 or below would need to have "0.8 protocol" module jar in
classpath in order to perform the upgrade.
* Client code
** jca and qpid-jms-amqp-0-x-test-utils folders have not been removed
** {{org.apache.qpid.common.ServerPropertyNames}} contains unused constants
** org.apache.rat plugin and org.jacoco plugin are declared twice in parent
pom.xml (once in plugins and second time in reporting) Is it necessary?
** It make sense to move "client/example" from client into one level up to make
it sibling of client.
** {{QpidByteBuffer}} can be removedr from client codebase. It does not bring
any benefits to the client as it simply wraps heap byte buffer. Thus, calls to
{{QpidByteBuffer}} can be substituted with calls to corresponding methods of
java heap {{ByteBuffer}}. Removal of {{QpidByteBuffer}} would make client code
simpler.
** It seems we can delete server command handlers and server related classes
like {{ServerDelegate}} (or move it into test). That might require deletion of
all methods {{#process(final QpidByteBuffer buffer, final
ServerChannelMethodProcessor dispatcher)}} from AMQMethodBody implementation
classes. After that ServerChannelMethodProcessor, ServerMethodProcessor can be
removed. However, we might want to change major version to 7.0 for these
changes. To be fair I would change version to 7.0 anyway as a structure of
client is changed and common dependency disappeared.
> Separate Qpid Broker for Java and 0-x JMS Client in source tree
> ---------------------------------------------------------------
>
> Key: QPID-7622
> URL: https://issues.apache.org/jira/browse/QPID-7622
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker, Java Client
> Reporter: Keith Wall
> Assignee: Keith Wall
> Fix For: qpid-java-client-0-x-6.3.0, qpid-java-broker-7.0.0
>
>
> As discussed here:
> http://qpid.2158936.n2.nabble.com/DISCUSS-Drop-the-AMQP-0-x-client-from-the-Qpid-for-Java-7-0-release-td7657770.html
> The proposal is to move the code and documentation that comprises the 0-x
> client to its own SVN root:
> https://svn.apache.org/repos/asf/qpid/qpid-jms-0-x
> The Java Broker and integration tests suites will remain at:
> https://svn.apache.org/repos/asf/qpid/qpid-java
> Maven dependencies will be used to pull in the appropriate 0-x client for
> integration testing purposes (as we already do with the Qpid JMS Client).
> The {{qpid-common}} module (and maven release artefact) will be eliminated.
> * Classes that the Broker requires will be moved into the class hierarchy of
> the existing plugin modules {{amqp-0-10-protocol}} and {{amqp-0-8-protocol}}
> e.g. {{org/apache/qpid/server/protocol/vx_y}}.* There is some generic code
> used by many modules such as BindingURL whose copy shall live in broker-core.
> * Code that the 0-x client requires will be copied into the client module.
> The package names will be unchanged.
> This will allow the Broker and 0-x Client to co-exist in the same JVM without
> class loading collision. Class movements will be organised in such a way to
> preserve SVN history.
> The structure of trunk qpid-jms-0-x will look like:
> - qpid-jms-0-x
> + client
> + example
> + jaa
> + example
> + ra
> + docs
> + jms-client-0-10
> + jms-client-0-8
> There will need to be a new parent POM.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
