[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127710#comment-13127710
]
jirapos...@reviews.apache.org commented on QPID-3401:
-----------------------------------------------------
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
line 976
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line976>
bq. >
bq. > This new isTopic(Destination) call doesnt seem like an improvement.
If a destination is a Topic, then it should implement the Topic interface and
the existing check is sufficient.
bq. >
bq. > Looking at the implementation of the isTopic method in
AMQDestination, it seems questionable since its perfectly possible for queues
bound to amq.topic not to be getting consumed from as JMS Topics.
bq. > If a destination is a Topic, then it should implement the Topic interface
and the existing check is sufficient.
Rajith: This is actually not true. When a destination is created using a
String, you cannot figure out if it's a "Topic" or a "Queue" until it's been
resolved.
So we can only return an Object implementing the javax.jms.Destination !
Please note the existing destination implementation is not entirely correct in
it's interpretation of the concept of Topics and Queues as it's quite narrow.
Simply bcos a queue is bound to amq.direct does not make it a "Queue' nor does
it make it a Topic bcos it's bound to 'amq.topic'.
bq. > Looking at the implementation of the isTopic method in AMQDestination, it
seems questionable since its perfectly possible for queues bound to amq.topic
not to be getting consumed from as JMS Topics.
The AddressBasedDestination overrides the isTopic() and isQueue() methods to
identify if address resolves to a Queue or a Topic.
What's missing is that if the underlying QpidDestination is null it should
resolve it and provide the correct answer.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
lines 1037-1059
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1037>
bq. >
bq. > Is this condition being used as an equivelent to if(BURL)else(ADDR)?
bq. >
bq. > This looks like it is in need of some abstraction.
It is infact the same. I used AMQTopic and AddressBasedTopic to make it more
explicit.
However I dislike the whole thing as I believe we need a more unified
implementation w.r.t Destinations.
As mentioned I didn't go as far I could have, but on the hand I am not sure how
far we can go either with the current code base.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
lines 1110-1121
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1110>
bq. >
bq. > Repeated if(AMQTopic)else() conditions, could use abstraction.
bq. >
bq. > Can the existing methods for exchangeName and queueName be overriden
in the AddressBasedTopic implementation?
I've since done that, but forgot to add it there. The AddressBasedDestination
does have these method overriden.
I will update the patch.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
lines 1250-1284
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line1250>
bq. >
bq. > Candidate for some sort of Destination factory?
Certainly !
The goal was to have minimal impact on the respective AMQxxx classes during the
first phase.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java,
line 2515
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49768#file49768line2515>
bq. >
bq. > The old way seems better. If I add another Topic implementation
later on, I'd now have to track this line down and update it.
correct ! The idea was to restrict Topics to the two said implementations to
keep it simple until we sort out a proper abstraction at the AMQxxx class level.
I'd expect changes that should make most of the above code obsolete.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 149
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line149>
bq. >
bq. > This doesnt seem to be used?
correct - forgot to remove this.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 580-588
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line580>
bq. >
bq. > Why does this catch an Exception now when it didnt before? Also,
Exception rather than something more specific?
The underlying address based implementation could throw an exception here.
As for the exception handling and other changes, I do have further improvements
in mind.
This patch is the beginning of a series of improvements I have in mind.
I want to take a step by step approach.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 598
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line598>
bq. >
bq. > As per previous comments on the isTopic() changes in AMQSession,
this condition seems questionable. If a Topic destination wasnt provided, then
overriding what it is classed as based on the exchange in use seems invalid.
Also, this looks to clash with logic used in BasicMessageConsumer which decides
the value of preAcquire in a different way (which is in itself a bug we were
already aiming to fix by consolidating those types of decision into 1 place).
This was existing code which I left as it is.
Perhaps I missed your point ?
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 1082
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49769#file49769line1082>
bq. >
bq. > It seems like at least some of these removed methods were better
placed where they were.
bq. >
bq. > It doesnt seem unreasonable for the protocol-specific implementation
of operations depending solely on the session to actually be in the
protocol-specific Session implementations. Better here than in the Destination
as some of the functionality now appears to be.
I provided an explanation for this in the design discussion part.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
line 536
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49770#file49770line536>
bq. >
bq. > All the casting here seems ugly, and this is an operation ultimately
performed by the session so it would seem cleaner if its implementation was
ultimately part of the session.
The ugly casting here is due to the lack of a proper abstraction.
I have commented on the above and the specifics about Destinations being smart
objects under the design discussion.
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java,
line 173
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line173>
bq. >
bq. > Whitespace error introduced (along with a vast number of others)
despite no other change around this code.
Oh dear ! I have not being able to get rid of this issue in eclipse :(
bq. On 2011-10-14 15:09:04, Robbie Gemmell wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java,
lines 229-230
bq. > <https://reviews.apache.org/r/2364/diff/1/?file=49771#file49771line229>
bq. >
bq. > TODO ?
Yes, correct. I wanted to achieve the same here without the destination object
having to leak address specific details outside of the abstraction.
I wasn't planning to commit with these TODO items and the durable subscription
stuff.
The aim was to attach a subsequent patch this weekend and put up what I had for
review as early as possible.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
On 2011-10-12 21:09:40, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2364/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-12 21:09:40)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and
Keith Wall.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. The following is a patch that illustrates the changes made to the core
client namely the session, message consumer and producer classes.
bq. (Please note that in order to compile and run the tests you need to get
apply the QPID-3401.patch attached to the JIRA.)
bq.
bq. Most of the code removed from the AMQSession_0_10.java have been included
in the new class structure posted as a separate review [
https://reviews.apache.org/r/2366/ ] to ensure clarity.
bq.
bq. In summary the changes are,
bq. 1. The code now uses AddressBasedDestination if the syntax is ADDR.
bq. 2. For address destinations the code now delegates the creation,
assertion, deletion actions to the underlying QpidDestination class via the
AddressBasedDestination.
bq. 3. The code also delegates creating of subscriptions.
bq.
bq. TODO.
bq. 1. Delegate the deleting of subscriptions (minor change which will follow
once this patch is approved)
bq. 2. Currently Durable Subscribers want work with AddressBasedDestinations
(This will be done in a follow up patch that will be posted soon).
bq.
bq. (AddressBasedDestination, AddressBasedTopic and AddressBasedQueue classes
are included along with the new class structure patch as a separate review).
bq.
bq.
bq. This addresses bug QPID-3401.
bq. https://issues.apache.org/jira/browse/QPID-3401
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
1182391
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1182391
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1182391
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
1182391
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/message/AMQMessageDelegate_0_10.java
1182391
bq.
bq. Diff: https://reviews.apache.org/r/2364/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All existing tests in AddressBasedDestination test pass (with the
exception of the Durable subscription test).
bq.
bq.
bq. Thanks,
bq.
bq. rajith
bq.
bq.
> Refactor address resolution code
> --------------------------------
>
> Key: QPID-3401
> URL: https://issues.apache.org/jira/browse/QPID-3401
> Project: Qpid
> Issue Type: Improvement
> Components: Java Client
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
> Fix For: 0.14
>
> Attachments: QPID-3401-systests.patch, QPID-3401.patch,
> class_diagram.png, model2.gif
>
>
> After some thought it seems that the following JIRA's would benefit from some
> reworking of the address resolution code as the original design had a few
> flaws based on incorrect understanding of the address syntax.
> QPID-3265
> QPID-3317
> QPID-3271
> The redesign would be minimal and not very disruptive. The goal is to fix
> certain design flaws in the current code, rather than a complete redesign. I
> am planning to reuse as much code as possible to ensure we don't throw away
> tested code.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
