[
https://issues.apache.org/jira/browse/QPID-3401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13127610#comment-13127610
]
[email protected] commented on QPID-3401:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2364/#review2584
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5792>
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.
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.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5793>
Is this condition being used as an equivelent to if(BURL)else(ADDR)?
This looks like it is in need of some abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5794>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5795>
Repeated if(AMQTopic)else() conditions, could use abstraction.
Can the existing methods for exchangeName and queueName be overriden in the
AddressBasedTopic implementation?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5796>
Is the Subject used as the queueName for AddressBasedTopics? I thought it
was used as the routing key?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5797>
Repeated if(AMQTopic)else() conditions, could use abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5798>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5799>
Candidate for some sort of Destination factory?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5800>
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.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
<https://reviews.apache.org/r/2364/#comment5801>
See earlier comment on this.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5802>
This doesnt seem to be used?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5803>
Why does this catch an Exception now when it didnt before? Also, Exception
rather than something more specific?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5807>
The ever-maintainable if-else pattern. Abstraction required.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5804>
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).
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5806>
The Destination being an instance of Topic would seem to be a more robust
check here. We should always make sure that is true for all our Topic
destination Objects.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/2364/#comment5809>
It seems like at least some of these removed methods were better placed
where they were.
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.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5810>
Yet more if(ADDR), needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5811>
As before, why do we now throw an exception, and why do we need to catch
all Exceptions here ?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
<https://reviews.apache.org/r/2364/#comment5812>
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.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5814>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5813>
Whitespace error introduced (along with a vast number of others) despite no
other change around this code.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5815>
Part of the ever-present if-else pattern, needs abstraction.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
<https://reviews.apache.org/r/2364/#comment5816>
TODO ?
- Robbie
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
>
>
> 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:[email protected]
