[
https://issues.apache.org/jira/browse/QPID-7658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15950830#comment-15950830
]
Lorenz Quack edited comment on QPID-7658 at 3/31/17 1:08 PM:
-------------------------------------------------------------
Thanks for the review. I know this was a larger piece of work with lots of code
moving around which is always difficult to review.
* {{NamedAddressSpace#getSendingLink(String, String)}} and
{{NamedAddressSpace#getReceivingLink(String, String)}}: I prefer having
separate functions rather than a boolean switch. Furthermore it would not
eliminate the if-else-block it would simply move it somewhere else. Therefore I
intend not to action this point.
* {{AMQPSession#doOnIOThreadAsync}}. Yes this was intentional since there is
nothing protocol specific about this and I thought other protocols might find
this useful. I also think reducing the differences between the protocol
implementations is desirable. No action.
* {{LinkImpl#attach}} precondition: No, the condition is correct. The {{_role}}
is the local role whereas {{attach.getRole}} returns the remote role. So if the
remote peer and the local peer have the same role something is wrong. No action.
* I'll remove the {{null}} check and CSRE and modify the {{createLinkEndpoint}}
method to ensure it always returns a non-null LinkEndpoint.
* You are right, the {{attach}} and link stealing is currently racey. I suggest
fixing that as part of QPID-7659
* Regarding removing {{ErrantLinkEndpoint}} in favour of {{hasError}}. I
preferred it this way since this is not a generic error reporting facility but
only used if the Attach fails to ensure the Link is immediately Detached.
However, I'll remove the {{attachWasUnsuccessful}}. It will be the
responsibility of the Link#attach to throw an exception in case of an error.
* I'll remove the unused variables you mentioned.
* I'll rename {{ReceivingLinkEndpoint}} to {{AbstractReceivingLinkEndpoint}}
* I'll fix {{SendingLinkEndpoint#remoteDetachedPerformDetach}}
* I'll change the error message in {{Session_1_0#receiveAttach}}
* Making {{Link_1_0}} and {{LinkEndpoint}} generic is a larger patch. I'll
commit that separately.
was (Author: lorenz.quack):
* {{NamedAddressSpace#getSendingLink(String, String)}} and
{{NamedAddressSpace#getReceivingLink(String, String)}}: I prefer having
separate functions rather than a boolean switch. Furthermore it would not
eliminate the if-else-block it would simply move it somewhere else. Therefore I
intend not to action this point.
* {{AMQPSession#doOnIOThreadAsync}}. Yes this was intentional since there is
nothing protocol specific about this and I thought other protocols might find
this useful. I also think reducing the differences between the protocol
implementations is desirable. No action.
* {{LinkImpl#attach}} precondition: No, the condition is correct. The {{_role}}
is the local role whereas {{attach.getRole}} returns the remote role. So if the
remote peer and the local peer have the same role something is wrong. No action.
* I'll remove the {{null}} check and CSRE and modify the {{createLinkEndpoint}}
method to ensure it always returns a non-null LinkEndpoint.
* You are right, the {{attach}} and link stealing is currently racey. I suggest
fixing that as part of QPID-7659
* Regarding removing {{ErrantLinkEndpoint}} in favour of {{hasError}}. I
preferred it this way since this is not a generic error reporting facility but
only used if the Attach fails to ensure the Link is immediately Detached.
However, I'll remove the {{attachWasUnsuccessful}}. It will be the
responsibility of the Link#attach to throw an exception in case of an error.
* I'll remove the unused variables you mentioned.
* I'll rename {{ReceivingLinkEndpoint}} to {{AbstractReceivingLinkEndpoint}}
* I'll fix {{SendingLinkEndpoint#remoteDetachedPerformDetach}}
* I'll change the error message in {{Session_1_0#receiveAttach}}
* Making {{Link_1_0}} and {{LinkEndpoint}} generic is a larger patch. I'll
commit that separately.
> [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]
