Return-Path: X-Original-To: apmail-camel-dev-archive@www.apache.org Delivered-To: apmail-camel-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 11F7C11D45 for ; Tue, 15 Apr 2014 21:14:11 +0000 (UTC) Received: (qmail 23165 invoked by uid 500); 15 Apr 2014 21:14:10 -0000 Delivered-To: apmail-camel-dev-archive@camel.apache.org Received: (qmail 23101 invoked by uid 500); 15 Apr 2014 21:14:10 -0000 Mailing-List: contact dev-help@camel.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@camel.apache.org Delivered-To: mailing list dev@camel.apache.org Received: (qmail 23093 invoked by uid 99); 15 Apr 2014 21:14:10 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 15 Apr 2014 21:14:10 +0000 X-ASF-Spam-Status: No, hits=1.3 required=5.0 tests=SPF_PASS,URI_HEX X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: local policy includes SPF record at spf.trusted-forwarder.org) Received: from [216.139.236.26] (HELO sam.nabble.com) (216.139.236.26) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 15 Apr 2014 21:14:06 +0000 Received: from [192.168.236.26] (helo=sam.nabble.com) by sam.nabble.com with esmtp (Exim 4.72) (envelope-from ) id 1WaAfn-0001Qn-5Q for dev@camel.apache.org; Tue, 15 Apr 2014 14:13:43 -0700 Date: Tue, 15 Apr 2014 14:13:43 -0700 (PDT) From: Babak Vahdat To: dev@camel.apache.org Message-ID: <1397596422848-5750268.post@n5.nabble.com> In-Reply-To: References: <1397554426774-5750234.post@n5.nabble.com> <1397571461599-5750247.post@n5.nabble.com> Subject: Re: About a bug of the camel-mongodb component... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Virus-Checked: Checked by ClamAV on apache.org Well, I don't see any extra advantage when using ReadPreference#[primary/secondary/nearest/etc.] utilities compared to ReadPreference#valueOf() as: - ReadPreference#valueOf() is also part of the public API, the same as ReadPreference#[primary/secondary/nearest/etc.] API - ReadPreference#[primary/secondary/nearest/etc.] API also lacks a proper Javadoc :( The patch has been already applied to the branches and the documentation is now reflecting this fix. Would you please mind to take a look at another ticket I've raised as well and maybe comment directly into that ticket? https://issues.apache.org/jira/browse/CAMEL-7370 Also feel free to mark it as "Won't Fix" if you've doubts about that. Thanks! Babak Raul Kripalani wrote > Hi Babak, >=20 > You're right. On revisiting this logic it seems like this functionality > was > indeed unfinished. Even in the case where the Read Preference could be > resolved, we would still throw an exception. > I don't think there's much discussion here. Backwards-compatibility is no= t > beneficial here because it means non-working functionality. I highly doub= t > anybody is using this functionality of the component. >=20 > Therefore +0.5 to your change. The reason I don't go full +1 is because > the > Javadoc does not make any statements regarding how the > ReadPreference#valueOf(String) method actually resolves the read > preference. And ReadPreference is not an Enum, so it's not like the > behaviour is defined by the Java Specs anyway. They try to mimic Enums > without explaining it. >=20 > In a nutshell: by using this method, we make ourselves fragile to > undocumented implementation changes inside that method. >=20 > An alternative approach would be to use reflection and invoke the > appropriate ReadPreference#[primary/nearest/etc.] method to obtain the > ReadPreference. At least that's part of the public API and implicitly > documented by Javadocs, so it cannot be such an easy target for future > sneaky changes by the Mongo team. >=20 > Regards, >=20 > *Ra=C3=BAl Kripalani* > Apache Camel PMC Member & Committer | Enterprise Architect, Open Source > Integration specialist > http://about.me/raulkripalani | http://www.linkedin.com/in/raulkripalani > http://blog.raulkr.net | twitter: @raulvk >=20 > On Tue, Apr 15, 2014 at 3:17 PM, Babak Vahdat > < > babak.vahdat@ > >wrote: >=20 >> Hi Ra=C3=BAl, >> >> Thanks a lot for your feedback but it's still not clear to me how this >> URI >> option used to work properly in the previous versions as from very first >> day >> (Camel 2.10.0) the preexisting logic used to throw an >> IllegalArgumentException *unconditionally* in all cases at the end of th= e >> method, see: >> >> >> https://github.com/apache/camel/commit/096c866aec42fec18b9eddb5809a24e27= e43f902#diff-7744e9b913507ceca68769e9acd928c2R343 >> >> See also this fix from the last weekend: >> >> >> https://github.com/apache/camel/commit/d7347cec8859ef7142d5017f4d839c7fc= 9478ffc#diff-7744e9b913507ceca68769e9acd928c2 >> >> The reason why I'm insisting on this is because I want to understand if >> we >> would have enough good reasons to *backport* this fix (which "would >> break" >> the backward compatibility of this option, meaning the possible values t= o >> be >> passed for this option, e.g. "readPreference=3DprimaryPreferred=E2=80=9D= instead of >> >> "readPreference=3Dcom.mongodb.TaggableReadPreference$PrimaryPreferredRea= dPreference" >> etc.). >> >> In the meanwhile I've commited the fix *only* on the master branch but >> would >> need to get this fixed on camel-2.13.x as well as 2.12.x branches. >> >> Also both Camel 2.13.x as well as 2.12.x branches make use of the driver >> version 2.11.4 which provides the ReadPreference#valueOf() utility we >> would >> need for this change, so technically the way is open for backporting the >> fix, see also this commit: >> >> >> https://github.com/mongodb/mongo-java-driver/commit/222d660e33015bce54b0= 8845000c2ea77b944b32#diff-27c3bd4e56af669071c576ccec394864R227 >> >> Babak >> >> >> Raul Kripalani wrote >> > Hello Babak, >> > >> > Thanks for pointing this out! >> > >> > Just some background... The ReadPreference class in the MongoDB Java >> API >> > has changed substantially between the driver versions: >> > >> > * The initial contribution of camel-mongodb was done against driver >> > version >> > 2.7.3 [1]. >> > * Currently we are on driver version 2.12.0 in Camel 2.14-SNAPSHOT [2]= . >> > >> > The logic to resolve the Read Preference by reflection makes sense wit= h >> > driver version 2.7.3, but not with 2.12.0. >> > >> > I do remember testing this logic manually when I first contributed the >> > component. But clearly I missed making a unit test, which would have >> > helped >> > us detect this non-backwards compatible change. >> > >> > +1 to your adjustment proposal. >> > >> > [1] http://api.mongodb.org/java/2.7.3/com/mongodb/ReadPreference.html >> > [2] http://api.mongodb.org/java/2.12/com/mongodb/ReadPreference.html >> > >> > Regards, >> > >> > *Ra=C3=BAl Kripalani* >> > Apache Camel PMC Member & Committer | Enterprise Architect, Open Sourc= e >> > Integration specialist >> > http://about.me/raulkripalani | >> http://www.linkedin.com/in/raulkripalani >> > http://blog.raulkr.net | twitter: @raulvk >> > >> > On Tue, Apr 15, 2014 at 10:33 AM, Babak Vahdat >> > < >> >> > babak.vahdat@ >> >> > >wrote: >> > >> >> Hi >> >> >> >> There was a flaw by MongoDbEndpoint#setReadPreference() I tried to fi= x >> >> last >> >> week. See also the documentation about the readPreference option: >> >> >> >> https://camel.apache.org/mongodb.html >> >> >> >> In the meanwhile I've noticed that this option doesn't work *at all* >> >> anyway >> >> because: >> >> >> >> - For example if you try to use >> >> "readPreference=3Dcom.mongodb.ReadPreference$PrimaryReadPreference=E2= =80=9D the >> >> reflection hack will not work (with the current logic) as the static >> >> member >> >> class PrimaryReadPreference is private anyway (one would end up with >> >> IllegalAccessException etc.) >> >> - One can't for example make use of the value >> >> "com.mongodb.ReadPreference$TaggedReadPreference" (as the >> documentation >> >> says) because this preference has no default constructor! >> >> >> >> We also don't have any test case in the code base about this option. >> >> >> >> As this option doesn't work anyway, my suggestion is to change the >> >> possible >> >> values for this option to the ones >> com.mongodb.ReadPreference#valueOf() >> >> allows. I've raised a JIRA including a proposed fix for this: >> >> >> >> https://issues.apache.org/jira/browse/CAMEL-7369 >> >> >> >> I also think an option value like "readPreference=3DprimaryPreferred" >> would >> >> read much easier than >> >> >> >> >> "readPreference=3Dcom.mongodb.TaggableReadPreference$PrimaryPreferredRea= dPreference" >> >> etc. >> >> >> >> The price we would pay for this would be non-backward compatibility >> with >> >> the >> >> current behaviour but as the current implementation doesn't work >> anyway, >> >> I >> >> guess this's not an issue at all. >> >> >> >> Thoughts? >> >> >> >> Babak >> >> >> >> >> >> >> >> -- >> >> View this message in context: >> >> >> http://camel.465427.n5.nabble.com/About-a-bug-of-the-camel-mongodb-compo= nent-tp5750234.html >> >> Sent from the Camel Development mailing list archive at Nabble.com. >> >> >> >> >> >> >> >> -- >> View this message in context: >> http://camel.465427.n5.nabble.com/About-a-bug-of-the-camel-mongodb-compo= nent-tp5750234p5750247.html >> Sent from the Camel Development mailing list archive at Nabble.com. >> -- View this message in context: http://camel.465427.n5.nabble.com/About-a-bug= -of-the-camel-mongodb-component-tp5750234p5750268.html Sent from the Camel Development mailing list archive at Nabble.com.