qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: JMS client doesn't distinguish between node and link bindings.
Date Tue, 09 Oct 2012 02:26:09 GMT


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > 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?

Thanks a lot for the comments. I forgot to add the unit test file.
Let me first reply to your comments and add the file later.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 1085-1088
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1085>
> >
> >     You can always put //TODO: delete to let your IDE remind you to remove stuff
like this.

Ah, this somehow escaped my attention. Will remove this. Thx.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 1350-1358
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1350>
> >
> >     If its going to be a durable queue should it have a name called TempQueue? What
are the ways such a combination could occur?

The same behavior was there before as well, just that it wasn't obvious bcos it happened when
we called send0_10QueueDeclare.

To answer your question, according to the address syntax, a link could be marked as durable,
which will result in the subscription queue being declared durable if it's a Topic.
If somebody does that then they should probably also provide a name via the 'name' property
to avoid the above situation.
I agree that it's not a good combination, but it's possible as it stands.

Perhaps what we could do is to generate a warning asking them to consider naming the queue.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 1362-1365
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1362>
> >
> >     Do we need to add it if it is false?

If the address doesn't specifiy no-local then we will default to the no-local value specified
when creating the consumer.
The noLocal value here is a parameter passed all the way from AMQSession.java


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 1381
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1381>
> >
> >     Might as well fix the method names while you are in here. I think someone maybe
even raised a JIRA about it recently.

Mea culpa. Will fix this.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 1493-1496
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1493>
> >
> >     Should we add it if its false?

If the address doesn't specifiy no-local then we will default to the no-local value specified
when creating the consumer.
The noLocal value here is a parameter passed all the way from AMQSession.java


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 1531
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1531>
> >
> >     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)

So the bindings (whether specified in node or link properties) are to do *extra customization*
for the respective queue.

If the type is Queue, then the "queue-name" used in the binding is set to the 'Queue Name'
given in the address (unless an explicit queue name is specified (possible but doesn't make
sense)).

Now comes the question of what exchange-name to use if an exchange-name is not given in the
address.
The queue is already bound to the namelessExchange (a.k.a defaultExchange) so there the extra
customization in the form of a binding means they want to bind it to some other exchange.
Besides, the namelessExchange uses the actual "queue-name" for routing. So there is no point
in specifying an extra binding here.
Here the user has specified a key (if not it's an error), therefore he intends to use another
exchange and we assume amq.topic if nothing is specified. 

This has been existing behavior as well and the C++ client and python client uses the same
if I'm not mistaken.

I agree with you that the variable name is a bit confusing here. Let me think of a better
name to avoid the confusion.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
lines 1534-1542
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1534>
> >
> >     We do this in two places (again below), might be worth a method extraction if
only so they are consistent.

Agreed.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 1553
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1553>
> >
> >     Missing if(_logger.isDebugEnabled).

Will add, thx for pointing it out.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 1566
> > <https://reviews.apache.org/r/7412/diff/1/?file=174495#file174495line1566>
> >
> >     Sincerely, your friend //TODO

:( Another miss.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java,
lines 256-259
> > <https://reviews.apache.org/r/7412/diff/1/?file=174501#file174501line256>
> >
> >     Maybe we should, but javadoc saying so on the public close method doesnt seem
that useful.

I will remove the java doc comment. It was more of a note to myself to further explore this
issue.
But it might be useful to have a proper comment so anybody revisting might get a reminder.


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java,
lines 268-287
> > <https://reviews.apache.org/r/7412/diff/1/?file=174501#file174501line268>
> >
> >     Doesnt look like anything has been done to prevent TransportExceptions being
propagated to this level, so I'm not sure that removal is wise.

Fair comment. I will add it as another catch clause.
Btw this is why I insit on your reviewing. You rarely miss them :)


> On Oct. 7, 2012, 7:06 p.m., Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/client/destination/AddressBasedDestinationTest.java,
line 1316
> > <https://reviews.apache.org/r/7412/diff/1/?file=174505#file174505line1316>
> >
> >     The tests could use some javadoc to describe what they are doing and why.

Will do.


- rajith


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


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