> 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 <[email protected]> wrote: >> >>> On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell >>> <[email protected]>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 <[email protected]> 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 >>> > > >>> > >>> >> >> >
