qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robbie Gemmell" <robbie.gemm...@gmail.com>
Subject Re: Review Request: JMS client doesn't distinguish between node and link bindings.
Date Sun, 07 Oct 2012 19:06:24 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7412/#review12215
-----------------------------------------------------------


I have reviewed this mostly on a 'its just code' basis, because Im not sure I really know
precisely what its actually meant to do (after reading this review request, the original JIRA,
and our documentation on addresses).

I dont see the mentioned unit tests, did you miss out the relevant file?


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/7412/#comment25897>

    You can always put //TODO: delete to let your IDE remind you to remove stuff like 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/7412/#comment25899>

    If its going to be a durable queue should it have a name called TempQueue? What are the
ways such a combination could occur?



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/7412/#comment25898>

    Do we need to add it if it is false?



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/7412/#comment25901>

    Might as well fix the method names while you are in here. I think someone maybe even raised
a JIRA about it recently.



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/7412/#comment25900>

    Might as well fix the method names while you are in here. I think someone maybe evne raised
a JIRA about it recently.



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/7412/#comment25902>

    Should we add it if its false?



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/7412/#comment25905>

    This is a little confusing. If they are using Queues, which usually go through the nameless/default
exchange, why would we bind to amq.topic? How would things get sent to the broker that would
actually use this binding rather than use the queue name on the nameless/default exchange,
do people magically need to know to send to amq.topic?
    
    (Also, the variable name is a little weird, which is why I use the contrived nameless/default
format I did above for the default exchange)



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/7412/#comment25906>

    We do this in two places (again below), might be worth a method extraction if only so
they are consistent.



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/7412/#comment25907>

    Missing if(_logger.isDebugEnabled).



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/7412/#comment25903>

    Sincerely, your friend //TODO



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/7412/#comment25908>

    Message is wrong (clearly a C&P error from the above method. Given how these are almost
identical loops, perhaps a method is in order with a boolean paramter to decide between create
and delete).
    
    Also missing if(_logger.isDebugEnabled) by virtue of C&P.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java
<https://reviews.apache.org/r/7412/#comment25910>

    Sincerely, your friend //TODO



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/7412/#comment25911>

    Maybe we should, but javadoc saying so on the public close method doesnt seem that useful.



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/7412/#comment25913>

    Doesnt look like anything has been done to prevent TransportExceptions being propagated
to this level, so I'm not sure that removal is wise.



http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java
<https://reviews.apache.org/r/7412/#comment25914>

    The tests could use some javadoc to describe what they are doing and why.


- Robbie Gemmell


On Oct. 6, 2012, 4:55 p.m., rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7412/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2012, 4:55 p.m.)
> 
> 
> Review request for qpid, Robbie Gemmell, Weston Price, and Keith Wall.
> 
> 
> Description
> -------
> 
> The attached patch does the following,
> 1. Node bindings are created when the node is created (was there before).
> 2. Node bindings are deleted when the node is deleted (added in this patch).
> 3. Link bindings are created when a subscription or producer is created (was there before)
> 4. Link bindings are deleted when a subscription or producer is closed (added in this
patch).
> 
> The code was modified to treat node and link bindings separately.
> I have also added sync() methods (disregarding the nowait flag) when creating a producer
or consumer to ensure we report the errors correctly.
> (In the existing code, When creating consumers the nowait flag was set to false, while
when creating durable subscribers it was set to true.).
> 
> Please ignore the white space errors. The git based patch I have is free of them.
> I had to create an svn diff as rb doesn't accept the git patch.
> 
> 
> This addresses bug QPID-3317.
>     https://issues.apache.org/jira/browse/QPID-3317
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQDestination.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_8.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/AddressHelper.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/Link.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/messaging/address/Node.java
1394702 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java
1394702 
> 
> Diff: https://reviews.apache.org/r/7412/diff/
> 
> 
> Testing
> -------
> 
> Used the existing AddressBasedDestinationTest to verify the new work does not break any
functionality.
> Added an explicit test case for verifying link beahvior as well as customizing the subscription
queue.
> Added unit tests for AddressHelper.
> 
> 
> Thanks,
> 
> rajith attapattu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message