[
https://issues.apache.org/jira/browse/QPID-7633?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15839690#comment-15839690
]
Lorenz Quack edited comment on QPID-7633 at 1/27/17 11:16 AM:
--------------------------------------------------------------
review comments:
* {{AMQPSession#getBlocking}} seems to be redundant.
{{Session#isProducerFlowBlocked}} seems to already serve the same purpose. Same
goes for {{Session#getUnacknowledgedMessages}} and
{{Sesssion#getUnacknowledgedMessageCount}}.
* Is there a point in re-defining abstract methods in {{AbstractAMQPSession}}
if they are already on the interfaces? Affected (some of which have
{{@Override}} while others do not): {{getBlocking}}, {{getConsumers}},
{{getConsumerCount}}, {{getTxnRejects}}, {{getTxnCommits}}, {{getTxnStart}},
{{getUnacknowledgedMessageCount}}, {{doTimeoutAction}},
{{getTransactionUpdateTimeLong}}, {{getTransactionStartTimeLong}}, and
{{transportStateChanged}}.
* Why do we only return {{0}} or {{1}} from
{{AbstractAMQPSession#getLocalTransactionOpen}}. That seems wrong.
* {{Session_1_0#setReceivingChannel}} and {{Session_1_0#setSendingChannel}} can
die and the functionality should move to the constructor. In this case the
switch statements probably go away.
* -There seems to be a race condition in how we allocate the
{{sendingChannelId}} in {{AMQPConnection_1_0Impl#receiveBegin}}.-
* I think eventually {{ServerSession}} and {{ServerSessionDelegate}} should
die. I guess that this could potentially be done when we separate the client
from the broker but not now.
* {{AMQPConnection_1_0Impl}} should not be used other than for construction.
Instead we should be referring to the Interface. There are two places where it
is currently references where I think it shouldn't
** {{AMQPConnection_1_0Impl#SHARED_SUBSCRIPTIONS}} could move to the interface
or together with the {{AMQPConnection_1_0Impl#ANONYMOUS_RELAY}}
** {{FrameHandler}} wants a {{ConnectionHandler}} so instead of casting I think
we should have a {{getConnectionHandler}} on the Interface
* Since essentially {{AMQSessionModel}} has been renamed to {{AMQPSession}}
should the method {{Consumer#getSessionModel}},
{{ConsumerTarget#getSessionModel}}, and
{{ManagementNodeConsumer#getSessionModel}} also be renamed?
* there are also some leftover awkward local variable names:
** {{sessionModel}} in {{AbstractQueue#onOpen}}
** {{channel}} in {{AbstractQueue#checkCapacity(AMQPSession)}}
** {{blockedChannels}} as field and local var in
{{AbstractQueue#checkCapacity()}}
** {{sessionModel}} in {{QueueConsumerImpl#createAttributeMap}}
* I think we should replace {{AMQPConnection#getAggregateTicker}} with
{{AMQPConnection#addTicker}} and {{AMQPConnection#removeTicker}}. This would
have the benefit of increasing encapsulation and the Connection could take care
of waking itself up instead of the caller having to remember to call
{{notifyWork}}. Unfortunately, {{ProtocolEngine#getAggregateTicker}} seems to
have to stay for the moment.
* I think we should kill {{ConsumerTarget_0_10#getSession}} and replace with
{{ConsumerTarget_0_10#acknowledge}} which can then be invoked directly from
{{ExplicitAcceptDispositionChangeListener}} like it already happens for
{{release}} and {{reject}}
was (Author: lorenz.quack):
review comments:
* {{AMQPSession#getBlocking}} seems to be redundant.
{{Session#isProducerFlowBlocked}} seems to already serve the same purpose. Same
goes for {{Session#getUnacknowledgedMessages}} and
{{Sesssion#getUnacknowledgedMessageCount}}.
* Is there a point in re-defining abstract methods in {{AbstractAMQPSession}}
if they are already on the interfaces? Affected (some of which have
{{@Override}} while others do not): {{getBlocking}}, {{getConsumers}},
{{getConsumerCount}}, {{getTxnRejects}}, {{getTxnCommits}}, {{getTxnStart}},
{{getUnacknowledgedMessageCount}}, {{doTimeoutAction}},
{{getTransactionUpdateTimeLong}}, {{getTransactionStartTimeLong}}, and
{{transportStateChanged}}.
* Why do we only return {{0}} or {{1}} from
{{AbstractAMQPSession#getLocalTransactionOpen}}. That seems wrong.
* {{Session_1_0#setReceivingChannel}} and {{Session_1_0#setSendingChannel}} can
die and the functionality should move to the constructor. In this case the
switch statements probably go away.
* There seems to be a race condition in how we allocate the
{{sendingChannelId}} in {{AMQPConnection_1_0Impl#receiveBegin}}.
* I think eventually {{ServerSession}} and {{ServerSessionDelegate}} should
die. I guess that this could potentially be done when we separate the client
from the broker but not now.
* {{AMQPConnection_1_0Impl}} should not be used other than for construction.
Instead we should be referring to the Interface. There are two places where it
is currently references where I think it shouldn't
** {{AMQPConnection_1_0Impl#SHARED_SUBSCRIPTIONS}} could move to the interface
or together with the {{AMQPConnection_1_0Impl#ANONYMOUS_RELAY}}
** {{FrameHandler}} wants a {{ConnectionHandler}} so instead of casting I think
we should have a {{getConnectionHandler}} on the Interface
* Since essentially {{AMQSessionModel}} has been renamed to {{AMQPSession}}
should the method {{Consumer#getSessionModel}},
{{ConsumerTarget#getSessionModel}}, and
{{ManagementNodeConsumer#getSessionModel}} also be renamed?
* there are also some leftover awkward local variable names:
** {{sessionModel}} in {{AbstractQueue#onOpen}}
** {{channel}} in {{AbstractQueue#checkCapacity(AMQPSession)}}
** {{blockedChannels}} as field and local var in
{{AbstractQueue#checkCapacity()}}
** {{sessionModel}} in {{QueueConsumerImpl#createAttributeMap}}
* I think we should replace {{AMQPConnection#getAggregateTicker}} with
{{AMQPConnection#addTicker}} and {{AMQPConnection#removeTicker}}. This would
have the benefit of increasing encapsulation and the Connection could take care
of waking itself up instead of the caller having to remember to call
{{notifyWork}}. Unfortunately, {{ProtocolEngine#getAggregateTicker}} seems to
have to stay for the moment.
* I think we should kill {{ConsumerTarget_0_10#getSession}} and replace with
{{ConsumerTarget_0_10#acknowledge}} which can then be invoked directly from
{{ExplicitAcceptDispositionChangeListener}} like it already happens for
{{release}} and {{reject}}
> Remove SessionAdapter
> ---------------------
>
> Key: QPID-7633
> URL: https://issues.apache.org/jira/browse/QPID-7633
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Keith Wall
> Assignee: Keith Wall
> Fix For: qpid-java-7.0
>
> Attachments: 0001-QPID-7633-Java-Broker-Remove-SessionAdapter.patch,
> QPID-7633.tar.bz2
>
>
> {{SessionAdapter}} is legacy code that predates the existing model. It
> should be removed, allowing for better code reuse between session
> implementations and better unit/smoke tests.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
