You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Rajith Attapattu <ra...@gmail.com> on 2011/03/14 17:48:23 UTC
Request to include QPID-3143 in the 0.10 release
The attached patch in QPID-3143 contains a simple one line fix and a
modified test case to test the change.
The same is committed to trunk at rev 1081460.
The fix is low risk IMHO.
However the fix is important as it makes the session.createQueue method spec
complaint.
It also allows the default behaviour to be easily overridden if the user
chooses do so.
Regards,
Rajith
Re: Request to include QPID-3143 in the 0.10 release
Posted by Robbie Gemmell <ro...@gmail.com>.
On 15 March 2011 16:34, Rajith Attapattu <ra...@gmail.com> wrote:
>
>
> On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell <robbie.gemmell@gmail.com
> > wrote:
>
>> I feel that this change should not be included in the 0.10 release.
>>
>> I think we are too close to release (RC1 is due tomorrow yes?) and that
>> there isnt significant enough testing in this area to provide enough
>> confidence when making what is actually an appreciable change in client
>> behaviour at this stage.
>>
>>
> I actually disagree with your assessment.
> I certainly consider this a **very low risk** change, given that addressing
> is the *default*.
>
Addressing already being the default makes me less inclined to change it
than if it wasn't, not the other way around.
> The only difference here is that the create option is not selected and the
> impact of that is an exception will be thrown if the queue is not present.
>
> There is absolutely no other **extra** risk involved here.
> The risk you perceive here is the impact of the addressing implementation
> due to lack of tests which is a valid concern.
> However that risk is there whether or not this commit is included or not.
>
There is risk that this commit will introduce behaviour that we have not
accounted for, which can/will not show up in our test suite because we are
not testing the code in the manner that users will actually use it. Whether
the addressing implementation has pre-existing issues undetected by the
current tests is also unknown, but that is an entirely different concern
than this issue. There is a clearly seperate risk that making this change
will introduce unforseen issues.
> This commit does not include anymore risk in the client than what is
> already there due to addressing being the default.
> Does that make sense ? Do you agree with that ?
>
>
>> So far as I can tell from the quick look I've taken so far
>> AddressBasedDestinationTest is the only test which exercises the
>> Addressing
>> code to any extent, and does so often by prefixing the address strings
>> with
>> 'ADDR:' because the test profiles are all running set to use the old
>> BindingURL syntax. As a result it would seem we ultimately arent testing
>> the
>> precise execution paths for many operations users will undertake at all.
>>
>>
> This is one of the biggest obstacles I had when testing the new address
> based implementation.
> Switching the default will create many test failures that is impossible to
> handle on my own.
> Also switching the default is not a good solution either as we still
> support the older binding URL mechanism as well.
>
I had not fully appreciated the extent to which the BURL addressing was
being used in the testing profiles, and would not have been comfortable
making the switch in default addressing syntax for 0.8 had I known. We
absolutely need to be having the majority of our testing concentrated on the
default behaviour users will encounter as to do otherwise is somewhat silly,
and the fact we arent testing much of the default behaviour at all is fairly
ridiculous.
The addressing scheme actually exercised code paths in a certain way that it
> highlighted several race conditions, a few dead locks and many other errors
> that were there in the code base for a long time. Therefore it would have
> been nice had I been able to catch these issues using our test suites. Ex
> issues in durable subscribers, queue receivers etc..
>
> I also believe this highlights a weakness in a our test suite - all though
> this is separate discussion on it's own.
> We should probably try to use the JMS interfaces as much as possible when
> writing our integration tests and have enough provisions to abstract out as
> many details as possible.
> This allows us to easily configure a single test to run under different
> configurations.
>
> Rajith
>
> Robbie
>>
>> On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
>>
>> > The attached patch in QPID-3143 contains a simple one line fix and a
>> > modified test case to test the change.
>> > The same is committed to trunk at rev 1081460.
>> > The fix is low risk IMHO.
>> >
>> > However the fix is important as it makes the session.createQueue method
>> > spec
>> > complaint.
>> > It also allows the default behaviour to be easily overridden if the user
>> > chooses do so.
>> >
>> > Regards,
>> >
>> > Rajith
>> >
>>
>
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Robert Godfrey <ro...@gmail.com>.
> I actually consider this a bug as I didn't really implement what I should
> have in the first place.
> The current behaviour is neither backwards compatible nor correct as per
> the JMS spec.
>
>
I think talking about the JMS spec here is a bit of a Red Herring... The
change here is regarding the nature of the object returned by
createQueue(...). Both the "before" and "after" are compliant with the spec
as the spec treats nature of the object returned as completely opaque.
On could argue that being able to create and Destination Object which upon
creation of a producer or consumer which references the Destination causes
the an entity (Queue) to be created on the Broker as "non-compliant", or at
least not within the spirit of the JMS spec... but even after your change
this would still be the case if you include the necessary options in the
supplied address String.
As for your comments about the tests, I fully agree with you and have said
> the same in my response to Robbie with some examples.
>
> But as I mentioned, that risk is **already there in 0.10**
>
>
Clearly there is a risk in 0.10 (an in 0.8) and if I had properly
investigated at the time of the 0.8 release I would have argued that we
should not have changed the default without changing the tests - this was an
omission on my part which I apologies for. Having said that, my expectation
of a code freeze in anticipation of a release is that we should only be
committing urgent and necessary changes, or changes of a very low risk and
high utility. For me this sadly does not meet that bar (I do want to see
this behaviour in Qpid, as I've previously said).
> and this commit is not adding anything to it.
>
>
The problem is that we cannot properly prove that this is not increasing the
risk, as we have no tests that actually use the "default" behaviour... the
only tests explicitly use the ADDR syntax since we change the default in the
test runs to Binding URL.
I think this patch should be made to trunk immediately, and a new JIRA
should be raised to change the test runner so it uses the "default
default". As previously I would not have object to this in isolation if it
had been part of the original check-in... but we really should be trying to
maintain some discipline once we have announced a freeze.
I also recommend that we should probably have some sort of WARNING log
message generated on the client if they try to create a destination based on
a String which does not explicitly define which syntax it is using since we
keep changing the behaviour associated with this option. Users need to be
explicitly stating their intent so they are not surprised by our changes in
behaviour.
-- Rob
> Rajith
>
>
>> -- Rob
>>
>> On 15 March 2011 16:34, Rajith Attapattu <ra...@gmail.com> wrote:
>>
>>> On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell
>>> <ro...@gmail.com>wrote:
>>>
>>> > I feel that this change should not be included in the 0.10 release.
>>> >
>>> > I think we are too close to release (RC1 is due tomorrow yes?) and that
>>> > there isnt significant enough testing in this area to provide enough
>>> > confidence when making what is actually an appreciable change in client
>>> > behaviour at this stage.
>>> >
>>> >
>>> I actually disagree with your assessment.
>>> I certainly consider this a **very low risk** change, given that
>>> addressing
>>> is the *default*.
>>> The only difference here is that the create option is not selected and
>>> the
>>> impact of that is an exception will be thrown if the queue is not
>>> present.
>>>
>>> There is absolutely no other **extra** risk involved here.
>>> The risk you perceive here is the impact of the addressing implementation
>>> due to lack of tests which is a valid concern.
>>> However that risk is there whether or not this commit is included or not.
>>>
>>> This commit does not include anymore risk in the client than what is
>>> already
>>> there due to addressing being the default.
>>> Does that make sense ? Do you agree with that ?
>>>
>>>
>>> > So far as I can tell from the quick look I've taken so far
>>> > AddressBasedDestinationTest is the only test which exercises the
>>> Addressing
>>> > code to any extent, and does so often by prefixing the address strings
>>> with
>>> > 'ADDR:' because the test profiles are all running set to use the old
>>> > BindingURL syntax. As a result it would seem we ultimately arent
>>> testing
>>> > the
>>> > precise execution paths for many operations users will undertake at
>>> all.
>>> >
>>> >
>>> This is one of the biggest obstacles I had when testing the new address
>>> based implementation.
>>> Switching the default will create many test failures that is impossible
>>> to
>>> handle on my own.
>>> Also switching the default is not a good solution either as we still
>>> support
>>> the older binding URL mechanism as well.
>>>
>>> The addressing scheme actually exercised code paths in a certain way that
>>> it
>>> highlighted several race conditions, a few dead locks and many other
>>> errors
>>> that were there in the code base for a long time. Therefore it would
>>> have
>>> been nice had I been able to catch these issues using our test suites. Ex
>>> issues in durable subscribers, queue receivers etc..
>>>
>>> I also believe this highlights a weakness in a our test suite - all
>>> though
>>> this is separate discussion on it's own.
>>> We should probably try to use the JMS interfaces as much as possible when
>>> writing our integration tests and have enough provisions to abstract out
>>> as
>>> many details as possible.
>>> This allows us to easily configure a single test to run under different
>>> configurations.
>>>
>>> Rajith
>>>
>>> Robbie
>>> >
>>> > On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
>>> >
>>> > > The attached patch in QPID-3143 contains a simple one line fix and a
>>> > > modified test case to test the change.
>>> > > The same is committed to trunk at rev 1081460.
>>> > > The fix is low risk IMHO.
>>> > >
>>> > > However the fix is important as it makes the session.createQueue
>>> method
>>> > > spec
>>> > > complaint.
>>> > > It also allows the default behaviour to be easily overridden if the
>>> user
>>> > > chooses do so.
>>> > >
>>> > > Regards,
>>> > >
>>> > > Rajith
>>> > >
>>> >
>>>
>>
>>
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Justin Ross <jr...@redhat.com>.
On Tue, 15 Mar 2011, Robbie Gemmell wrote:
> On 15 March 2011 17:33, Rajith Attapattu <ra...@gmail.com> wrote:
>
>>
>>
>> On Tue, Mar 15, 2011 at 1:24 PM, Robert Godfrey <ro...@gmail.com>wrote:
>>
>>> I see both side here, I think the fundamental issue for me is that I
>>> hadn't previously picked up that when the change to the default addressing
>>> syntax was changed in the 0.8 release, the tests were made to explicitly run
>>> with the Binding URL as the default. As such we are not exposing the
>>> "default" that we are giving to users when we run the tests - thus the
>>> expected normal code path is not being exercised in our tests other than in
>>> the one test that Robbie highlighted (and this uses the explicit "ADDR:"
>>> syntax).
>>>
>>> As such I don't feel like simply running the tests gives me the confidence
>>> that this change will not have unforseen consequences. In truth I actually
>>> now feel quite nervous about what we did in 0.8. I strongly feel that we
>>> should update the tests to use the default (which is now ADDR) syntax -
>>> changing any test where the BindingURL is implicitly expected.
>>>
>>> Given that this is a "change" rather than a "bug fix", the fact that we
>>> are supposed to be in code freeze in preparation for release, and that I
>>> don't have confidence that we are adequately testing the "default" behaviour
>>> using the standard tests - I don't feel that it is appropriate to include in
>>> 0.10.
>>>
>>
>> I actually consider this a bug as I didn't really implement what I should
>> have in the first place.
>> The current behaviour is neither backwards compatible nor correct as per
>> the JMS spec.
>>
>> As for your comments about the tests, I fully agree with you and have said
>> the same in my response to Robbie with some examples.
>> But as I mentioned, that risk is **already there in 0.10** and this commit
>> is not adding anything to it.
>>
>> Rajith
>>
>
> In my mind this is a 'feature' change rather than bug fix as the patch would
> alter a behaviour of the client that was, correctly or otherwise, long since
> put there deliberately and which you preserved deliberately.
>
> You are correct that there is existing risk already in 0.10, however this is
> an additional change which would clearly introduce an additional risk of
> unforseen effects in its own right versus not including it. Not having
> enough confidence in the existing situation is no reason to throw caustion
> to the wind.
>
> Robbie
Thanks, Rob, Robbie, and Rajith, for the discussion.
Applying the "no regressions, even prospective ones" rule, I oppose this
change for 0.10.
To my impression, it blurs the feature/bug distinction, and I personally
accept it as a defect. That doesn't mean, however, that we should accept
it for 0.10. Since the fix also constitutes an important behavior change,
and since it poses a real risk of introducing new regressions, I think it
should wait for 0.12.
Justin
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Request to include QPID-3143 in the 0.10 release
Posted by Robbie Gemmell <ro...@gmail.com>.
On 15 March 2011 17:33, Rajith Attapattu <ra...@gmail.com> wrote:
>
>
> On Tue, Mar 15, 2011 at 1:24 PM, Robert Godfrey <ro...@gmail.com>wrote:
>
>> I see both side here, I think the fundamental issue for me is that I
>> hadn't previously picked up that when the change to the default addressing
>> syntax was changed in the 0.8 release, the tests were made to explicitly run
>> with the Binding URL as the default. As such we are not exposing the
>> "default" that we are giving to users when we run the tests - thus the
>> expected normal code path is not being exercised in our tests other than in
>> the one test that Robbie highlighted (and this uses the explicit "ADDR:"
>> syntax).
>>
>> As such I don't feel like simply running the tests gives me the confidence
>> that this change will not have unforseen consequences. In truth I actually
>> now feel quite nervous about what we did in 0.8. I strongly feel that we
>> should update the tests to use the default (which is now ADDR) syntax -
>> changing any test where the BindingURL is implicitly expected.
>>
>> Given that this is a "change" rather than a "bug fix", the fact that we
>> are supposed to be in code freeze in preparation for release, and that I
>> don't have confidence that we are adequately testing the "default" behaviour
>> using the standard tests - I don't feel that it is appropriate to include in
>> 0.10.
>>
>
> I actually consider this a bug as I didn't really implement what I should
> have in the first place.
> The current behaviour is neither backwards compatible nor correct as per
> the JMS spec.
>
> As for your comments about the tests, I fully agree with you and have said
> the same in my response to Robbie with some examples.
> But as I mentioned, that risk is **already there in 0.10** and this commit
> is not adding anything to it.
>
> Rajith
>
In my mind this is a 'feature' change rather than bug fix as the patch would
alter a behaviour of the client that was, correctly or otherwise, long since
put there deliberately and which you preserved deliberately.
You are correct that there is existing risk already in 0.10, however this is
an additional change which would clearly introduce an additional risk of
unforseen effects in its own right versus not including it. Not having
enough confidence in the existing situation is no reason to throw caustion
to the wind.
Robbie
Re: Request to include QPID-3143 in the 0.10 release
Posted by Rajith Attapattu <ra...@gmail.com>.
On Tue, Mar 15, 2011 at 1:24 PM, Robert Godfrey <ro...@gmail.com>wrote:
> I see both side here, I think the fundamental issue for me is that I hadn't
> previously picked up that when the change to the default addressing syntax
> was changed in the 0.8 release, the tests were made to explicitly run with
> the Binding URL as the default. As such we are not exposing the "default"
> that we are giving to users when we run the tests - thus the expected normal
> code path is not being exercised in our tests other than in the one test
> that Robbie highlighted (and this uses the explicit "ADDR:" syntax).
>
> As such I don't feel like simply running the tests gives me the confidence
> that this change will not have unforseen consequences. In truth I actually
> now feel quite nervous about what we did in 0.8. I strongly feel that we
> should update the tests to use the default (which is now ADDR) syntax -
> changing any test where the BindingURL is implicitly expected.
>
> Given that this is a "change" rather than a "bug fix", the fact that we are
> supposed to be in code freeze in preparation for release, and that I don't
> have confidence that we are adequately testing the "default" behaviour using
> the standard tests - I don't feel that it is appropriate to include in 0.10.
>
I actually consider this a bug as I didn't really implement what I should
have in the first place.
The current behaviour is neither backwards compatible nor correct as per the
JMS spec.
As for your comments about the tests, I fully agree with you and have said
the same in my response to Robbie with some examples.
But as I mentioned, that risk is **already there in 0.10** and this commit
is not adding anything to it.
Rajith
> -- Rob
>
> On 15 March 2011 16:34, Rajith Attapattu <ra...@gmail.com> wrote:
>
>> On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell
>> <ro...@gmail.com>wrote:
>>
>> > I feel that this change should not be included in the 0.10 release.
>> >
>> > I think we are too close to release (RC1 is due tomorrow yes?) and that
>> > there isnt significant enough testing in this area to provide enough
>> > confidence when making what is actually an appreciable change in client
>> > behaviour at this stage.
>> >
>> >
>> I actually disagree with your assessment.
>> I certainly consider this a **very low risk** change, given that
>> addressing
>> is the *default*.
>> The only difference here is that the create option is not selected and the
>> impact of that is an exception will be thrown if the queue is not present.
>>
>> There is absolutely no other **extra** risk involved here.
>> The risk you perceive here is the impact of the addressing implementation
>> due to lack of tests which is a valid concern.
>> However that risk is there whether or not this commit is included or not.
>>
>> This commit does not include anymore risk in the client than what is
>> already
>> there due to addressing being the default.
>> Does that make sense ? Do you agree with that ?
>>
>>
>> > So far as I can tell from the quick look I've taken so far
>> > AddressBasedDestinationTest is the only test which exercises the
>> Addressing
>> > code to any extent, and does so often by prefixing the address strings
>> with
>> > 'ADDR:' because the test profiles are all running set to use the old
>> > BindingURL syntax. As a result it would seem we ultimately arent testing
>> > the
>> > precise execution paths for many operations users will undertake at all.
>> >
>> >
>> This is one of the biggest obstacles I had when testing the new address
>> based implementation.
>> Switching the default will create many test failures that is impossible to
>> handle on my own.
>> Also switching the default is not a good solution either as we still
>> support
>> the older binding URL mechanism as well.
>>
>> The addressing scheme actually exercised code paths in a certain way that
>> it
>> highlighted several race conditions, a few dead locks and many other
>> errors
>> that were there in the code base for a long time. Therefore it would have
>> been nice had I been able to catch these issues using our test suites. Ex
>> issues in durable subscribers, queue receivers etc..
>>
>> I also believe this highlights a weakness in a our test suite - all though
>> this is separate discussion on it's own.
>> We should probably try to use the JMS interfaces as much as possible when
>> writing our integration tests and have enough provisions to abstract out
>> as
>> many details as possible.
>> This allows us to easily configure a single test to run under different
>> configurations.
>>
>> Rajith
>>
>> Robbie
>> >
>> > On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
>> >
>> > > The attached patch in QPID-3143 contains a simple one line fix and a
>> > > modified test case to test the change.
>> > > The same is committed to trunk at rev 1081460.
>> > > The fix is low risk IMHO.
>> > >
>> > > However the fix is important as it makes the session.createQueue
>> method
>> > > spec
>> > > complaint.
>> > > It also allows the default behaviour to be easily overridden if the
>> user
>> > > chooses do so.
>> > >
>> > > Regards,
>> > >
>> > > Rajith
>> > >
>> >
>>
>
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Robert Godfrey <ro...@gmail.com>.
I see both side here, I think the fundamental issue for me is that I hadn't
previously picked up that when the change to the default addressing syntax
was changed in the 0.8 release, the tests were made to explicitly run with
the Binding URL as the default. As such we are not exposing the "default"
that we are giving to users when we run the tests - thus the expected normal
code path is not being exercised in our tests other than in the one test
that Robbie highlighted (and this uses the explicit "ADDR:" syntax).
As such I don't feel like simply running the tests gives me the confidence
that this change will not have unforseen consequences. In truth I actually
now feel quite nervous about what we did in 0.8. I strongly feel that we
should update the tests to use the default (which is now ADDR) syntax -
changing any test where the BindingURL is implicitly expected.
Given that this is a "change" rather than a "bug fix", the fact that we are
supposed to be in code freeze in preparation for release, and that I don't
have confidence that we are adequately testing the "default" behaviour using
the standard tests - I don't feel that it is appropriate to include in 0.10.
-- Rob
On 15 March 2011 16:34, Rajith Attapattu <ra...@gmail.com> wrote:
> On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell
> <ro...@gmail.com>wrote:
>
> > I feel that this change should not be included in the 0.10 release.
> >
> > I think we are too close to release (RC1 is due tomorrow yes?) and that
> > there isnt significant enough testing in this area to provide enough
> > confidence when making what is actually an appreciable change in client
> > behaviour at this stage.
> >
> >
> I actually disagree with your assessment.
> I certainly consider this a **very low risk** change, given that addressing
> is the *default*.
> The only difference here is that the create option is not selected and the
> impact of that is an exception will be thrown if the queue is not present.
>
> There is absolutely no other **extra** risk involved here.
> The risk you perceive here is the impact of the addressing implementation
> due to lack of tests which is a valid concern.
> However that risk is there whether or not this commit is included or not.
>
> This commit does not include anymore risk in the client than what is
> already
> there due to addressing being the default.
> Does that make sense ? Do you agree with that ?
>
>
> > So far as I can tell from the quick look I've taken so far
> > AddressBasedDestinationTest is the only test which exercises the
> Addressing
> > code to any extent, and does so often by prefixing the address strings
> with
> > 'ADDR:' because the test profiles are all running set to use the old
> > BindingURL syntax. As a result it would seem we ultimately arent testing
> > the
> > precise execution paths for many operations users will undertake at all.
> >
> >
> This is one of the biggest obstacles I had when testing the new address
> based implementation.
> Switching the default will create many test failures that is impossible to
> handle on my own.
> Also switching the default is not a good solution either as we still
> support
> the older binding URL mechanism as well.
>
> The addressing scheme actually exercised code paths in a certain way that
> it
> highlighted several race conditions, a few dead locks and many other errors
> that were there in the code base for a long time. Therefore it would have
> been nice had I been able to catch these issues using our test suites. Ex
> issues in durable subscribers, queue receivers etc..
>
> I also believe this highlights a weakness in a our test suite - all though
> this is separate discussion on it's own.
> We should probably try to use the JMS interfaces as much as possible when
> writing our integration tests and have enough provisions to abstract out as
> many details as possible.
> This allows us to easily configure a single test to run under different
> configurations.
>
> Rajith
>
> Robbie
> >
> > On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
> >
> > > The attached patch in QPID-3143 contains a simple one line fix and a
> > > modified test case to test the change.
> > > The same is committed to trunk at rev 1081460.
> > > The fix is low risk IMHO.
> > >
> > > However the fix is important as it makes the session.createQueue method
> > > spec
> > > complaint.
> > > It also allows the default behaviour to be easily overridden if the
> user
> > > chooses do so.
> > >
> > > Regards,
> > >
> > > Rajith
> > >
> >
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Rajith Attapattu <ra...@gmail.com>.
On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell
<ro...@gmail.com>wrote:
> I feel that this change should not be included in the 0.10 release.
>
> I think we are too close to release (RC1 is due tomorrow yes?) and that
> there isnt significant enough testing in this area to provide enough
> confidence when making what is actually an appreciable change in client
> behaviour at this stage.
>
>
I actually disagree with your assessment.
I certainly consider this a **very low risk** change, given that addressing
is the *default*.
The only difference here is that the create option is not selected and the
impact of that is an exception will be thrown if the queue is not present.
There is absolutely no other **extra** risk involved here.
The risk you perceive here is the impact of the addressing implementation
due to lack of tests which is a valid concern.
However that risk is there whether or not this commit is included or not.
This commit does not include anymore risk in the client than what is already
there due to addressing being the default.
Does that make sense ? Do you agree with that ?
> So far as I can tell from the quick look I've taken so far
> AddressBasedDestinationTest is the only test which exercises the Addressing
> code to any extent, and does so often by prefixing the address strings with
> 'ADDR:' because the test profiles are all running set to use the old
> BindingURL syntax. As a result it would seem we ultimately arent testing
> the
> precise execution paths for many operations users will undertake at all.
>
>
This is one of the biggest obstacles I had when testing the new address
based implementation.
Switching the default will create many test failures that is impossible to
handle on my own.
Also switching the default is not a good solution either as we still support
the older binding URL mechanism as well.
The addressing scheme actually exercised code paths in a certain way that it
highlighted several race conditions, a few dead locks and many other errors
that were there in the code base for a long time. Therefore it would have
been nice had I been able to catch these issues using our test suites. Ex
issues in durable subscribers, queue receivers etc..
I also believe this highlights a weakness in a our test suite - all though
this is separate discussion on it's own.
We should probably try to use the JMS interfaces as much as possible when
writing our integration tests and have enough provisions to abstract out as
many details as possible.
This allows us to easily configure a single test to run under different
configurations.
Rajith
Robbie
>
> On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
>
> > The attached patch in QPID-3143 contains a simple one line fix and a
> > modified test case to test the change.
> > The same is committed to trunk at rev 1081460.
> > The fix is low risk IMHO.
> >
> > However the fix is important as it makes the session.createQueue method
> > spec
> > complaint.
> > It also allows the default behaviour to be easily overridden if the user
> > chooses do so.
> >
> > Regards,
> >
> > Rajith
> >
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Robbie Gemmell <ro...@gmail.com>.
I feel that this change should not be included in the 0.10 release.
I think we are too close to release (RC1 is due tomorrow yes?) and that
there isnt significant enough testing in this area to provide enough
confidence when making what is actually an appreciable change in client
behaviour at this stage.
So far as I can tell from the quick look I've taken so far
AddressBasedDestinationTest is the only test which exercises the Addressing
code to any extent, and does so often by prefixing the address strings with
'ADDR:' because the test profiles are all running set to use the old
BindingURL syntax. As a result it would seem we ultimately arent testing the
precise execution paths for many operations users will undertake at all.
Robbie
On 14 March 2011 16:48, Rajith Attapattu <ra...@gmail.com> wrote:
> The attached patch in QPID-3143 contains a simple one line fix and a
> modified test case to test the change.
> The same is committed to trunk at rev 1081460.
> The fix is low risk IMHO.
>
> However the fix is important as it makes the session.createQueue method
> spec
> complaint.
> It also allows the default behaviour to be easily overridden if the user
> chooses do so.
>
> Regards,
>
> Rajith
>
Re: Request to include QPID-3143 in the 0.10 release
Posted by Andrew Kennedy <an...@gmail.com>.
On 14 Mar 2011, at 21:41, Rajith Attapattu wrote:
> On Mon, Mar 14, 2011 at 5:33 PM, Andrew Kennedy
> <an...@gmail.com> wrote:
>> Ok,
>>
>> From line 1216 of the latest AMQSession.java we have:
>>
>>> if (queueName.indexOf('/') == -1 && queueName.indexOf(';') == -1)
>>> {
>>> DestSyntax syntax = AMQDestination.getDestType(queueName);
>>> if (syntax == AMQDestination.DestSyntax.BURL)
>>> {
>>> // For testing we may want to use the prefix
>>> return new AMQQueue(getDefaultQueueExchangeName(),
>>> new AMQShortString(AMQDestination.stripSyntaxPrefix
>>> (queueName)));
>>> }
>>> else
>>> {
>>> AMQQueue queue = new AMQQueue(queueName);
>>> return queue;
>>> }
>>> }
>>> else
>>> {
>>> return new AMQQueue(queueName);
>>> }
>>> }
Rajith,
I've had time to catch up on the complete session.createQueue
discussion, and noticed you aren't pushing this for 0.10 inclusion
anymore. I agree with Rob et al and yourself that we aren't testing
things correctly. As I understood things, the (current) incorrect
behaviour was added here:
Revision 964984
http://svn.apache.org/viewvc/qpid/trunk/qpid/java/client/src/main/
java/org/apache/qpid/client/AMQSession.java?
annotate=964984&diff_format=h&pathrev=1081460
Modified Fri Jul 16 23:50:13 2010 UTC (7 months, 4 weeks ago) by rajith
And was therefore present in 0.8, which I had not realised before. I
think making the change is the right thing to do, but only once we
have updated the default test profile and the tests accordingly. If
the behavioural change had been introduced *post* 0.8, I think there
would be a stronger case for inclusion.
We probably need to create profiles like this "{broker}-{addressing
format}-{amqp version}":
vm-addr-0.10 (default), vm-burl-0.10, java-addr-0.10, java-
burl-0.10, cpp-addr-0.10, etc.
Or, we could change the test profile mechanism, so you can specify
something like "-Dprofiles=java,burl,0.10,mina", with (sensible)
combinations of sub-profiles being valid for the system tests. The
default set should still be something like "vm,addr,0.10" though.
This is (fairly) easy with Maven ;)
>> [...] should we really not just change this to simply "return new
>> AMQQueue(queueName);" and then leave everything to the addressing
>> syntax logic in AMQBindingURL.java and friends instead? It looks
>> like it should just work, but I haven't tested it. Oh, right, we
>> get the defaultQueueExchangeName from the *connection* URL, I see.
>> Well, that's probably more difficult...
>
> The reason is that the old implementation uses the amq.direct as
> the exchange name (which is what returned by
> defaultQueueExchangeName.)
> The addressing syntax implementation just uses the default exchange
> (or the nameless exchange) when dealing with queues.
So, with BURL syntax, you can override the default exchange name on
the connection URL. But, using ADDR syntax for the destination, there
is no way of overriding the default exchange name, then, even if we
change it on the connection URL? Is this right? Or, would you be also
using an ADDR type URL for the connection in that case?
> Just wanted to make it obvious in the code. Perhaps I should add a
> comment there as well ?
Yes, good idea.
Andrew.
--
-- andrew d kennedy ? do not fold, bend, spindle, or mutilate ;
-- http://grkvlt.blogspot.com/ ? edinburgh : +44 7582 293 255 ;
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org