[
https://issues.apache.org/jira/browse/QPID-7658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15945038#comment-15945038
]
Alex Rudyy commented on QPID-7658:
----------------------------------
My review comments for commits [ https://svn.apache.org/r1785660 ], [
https://svn.apache.org/r1785976 ] and [ https://svn.apache.org/r1786358 ]:
* Methods {{NamedAddressSpace#getSendingLink(String, String)}} and
{{NamedAddressSpace#getReceivingLink(String, String)}} can be replaced with
{{NamedAddressSpace#getLink(String remoteContainerId, String linkName, boolean
isReceiving)}}. That would eliminate the need of if-else block in the
{{Session_1_0#receiveAttach}} and simplify the code.
* Method {{AMQPSession#doOnIOThreadAsync}} was added to public interface but so
far it is only required by AMQP 1.0 layer. Is it an intention to have this
method available for 0.x AMQP implementation?
* LinkImpl
** {{LinkImpl#attach}} precondition check below looks wrong:
{code}
if (_role == attach.getRole())
{
throw new AmqpErrorException(new Error(AmqpError.ILLEGAL_STATE, "Cannot
switch SendingLink to ReceivingLink and vice versa"));
}
{code}
Should it actually be {code}if (_role != attach.getRole()){code}?
I am not sure that CSRE is required to throw if link endpoint after creation is
null
{code}
_linkEndpoint = createLinkEndpoint(session, attach);
if (_linkEndpoint == null)
{
throw new ConnectionScopedRuntimeException(String.format(
"LinkEndpoint creation failed for attach: %s",
attach));
}
{code}
Would it be better to create an instance of {{ErrantEndpoint}} or a
{{StandardReceivingEndpoint}} with error when target is null? Although
ConnectionScopedRuntimeException is turned into ErrantEndpoint in catch block
and the behaviour seems like spec complient( As per spec "If no target is
specified on an incoming link, then there is no target currently attached to
the link. A link with no target will never permit incoming messages") I would
avoid throwing CSRE here as it would be reported as INTERNAL ERROR which is
wrong?
** {{#attach}}: Null check for _linkEndpoint and getting a session is racy as
_linkEndpoint can be nulified between check and call. I would consider saving
_linkEndpoint in local variable and using local variable where where it is
feasible. Alternatively, we can consider using atomic state variable in
LinkImpl and use compareAndSet where it is possible but that might complicate
code on stealing...
** {{LinkImpl#stealLink}}: it closes the link endpoint with 'close' flag set to
true as required by the spec. But the side effect of the linkendpoint close is
that {{Link_1_0#linkClosed()}} is called which deletes the link from the
registry and its store. The removal of link from registry does not look
nesessary to me.
* Class ErrantLinkEndpoint looks a bit artificial. Why we cannot have methods
{{hasError}} and getError on {{LinkEndpoint}} interface instead?
The code in {{Session_1_0$EndpointCreationCallback#success}}
{code}
if (endpoint instanceof ErrantLinkEndpoint)
{
endpoint.sendAttach();
((ErrantLinkEndpoint) endpoint).closeWithError();
}
else if (attachWasUnsuccessful(endpoint))
{
endpoint.sendAttach();
Error error = new Error();
error.setCondition(AmqpError.NOT_FOUND);
endpoint.close(error);
}
{code}
could be replaced as below
{code}
if (endpoint.hasError())
{
endpoint.sendAttach();
endpoint.close(enpoint.getError()); // or endpoint.close();
}
{code}
That would simplify the code.
* {{SendingLinkEndpoint}}
** {{SendingLinkEndpoint#_durability}} is unused
** {{SendingLinkEndpoint}} would be better to rename into
{{AbstractSendingLinkEndpoint}}.
** {{SendingLinkEndpoint#remoteDetachedPerformDetach}} there is a posibillity
of sending multiple detaches in sequence.
* {{ReceivingLinkEndpoint}}
** {{ReceivingLinkEndpoint#_remoteDrain}} is unused. Is it a duplicate of a
{{_drain}} field an {{AbstractLinkEndpoint}}?
** {{ReceivingLinkEndpoint#_drainLimit}} is unused. The field is used to
evaluate result returned from {{ReceivingLinkEndpoint#isDrained}} but it seems
the only place where it called is {{ReceivingLinkEndpoint#receiveFlow}}
followed by noop. It is unclear what interntions are behind the {{#isDrained}}
code and unused fields.
* Session_1_0#receiveAttach: Error message ("inputHandle of Attach already in
use: " + attach.toString()) can be changed to be the same as in
ConnectionScopedRuntimeException ("Input Handle '%d' already in use").
* I am wondering whether interfaces Link_1_0 and LinkEndpoint can be made
parameterised (Link_1_0<S extends BaseSource, T extends BaseTarget>
LinkEndpoint<S extends BaseSource, T extends BaseTarget>) and concreate
instances could use there own Source and Target implementations. That should
eliminate the need for casting.
> [Java Broker] Improve LinkRegistry
> ----------------------------------
>
> Key: QPID-7658
> URL: https://issues.apache.org/jira/browse/QPID-7658
> Project: Qpid
> Issue Type: Task
> Components: Java Broker
> Reporter: Lorenz Quack
> Fix For: qpid-java-broker-7.0.0
>
>
> Currently the AbstractVirtualHost has a Map<remoteContainerId, LinkRegistry>.
> The handling of the remoteContainerId should also be encapsulated in the
> LinkRegistry giving each VH only a single LinkRegistry.
> The LinkRegistry is responsible for ensuring Link uniqueness and persistence
> (separate JIRA).
> Furthermore, the following change to the LinkRegistry is proposed:
> * LinkRegistry.getSendvingLink(String remoteContrainerId, String linkName) ->
> Link
> * LinkRegistry.getReceivingLink(String remoteContrainerId, String linkName)
> -> Link
> These should always return a non-null Link. The caller (session) is
> responsible for checking that the link has valid Source and Targets.
> They also need to be thread-safe (e.g., two calls with the same arguments
> should return the same object).
> The LinkRegistry should also provide facilities to the Link to remove itself
> from the Registry (for example on Link close, or error) and to update the
> Termini upon resuming a Link.
> If possible this should not be part of the public API. possible API:
> * LinkRegistry.removeSendingLink(String localContainerId, String
> remoteContrainerId, String linkName)
> * LinkRegistry.removeReceivingLink(String localContainerId, String
> remoteContrainerId, String linkName)
> * LinkRegistry.updateLinkTermini(Link link, Source source, Target target)
> The implementation of which must be thread-safe because the LinkRegistry will
> be accessed on multiple threads concurrently.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
