Hi Babak, 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 not beneficial here because it means non-working functionality. I highly doubt anybody is using this functionality of the component.
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. In a nutshell: by using this method, we make ourselves fragile to undocumented implementation changes inside that method. 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. Regards, *Raúl 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 On Tue, Apr 15, 2014 at 3:17 PM, Babak Vahdat <babak.vah...@swissonline.ch>wrote: > Hi Raúl, > > 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 the > method, see: > > > https://github.com/apache/camel/commit/096c866aec42fec18b9eddb5809a24e27e43f902#diff-7744e9b913507ceca68769e9acd928c2R343 > > See also this fix from the last weekend: > > > https://github.com/apache/camel/commit/d7347cec8859ef7142d5017f4d839c7fc9478ffc#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 to > be > passed for this option, e.g. "readPreference=primaryPreferred” instead of > > "readPreference=com.mongodb.TaggableReadPreference$PrimaryPreferredReadPreference" > 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/222d660e33015bce54b08845000c2ea77b944b32#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 with > > 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úl 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 > > > > 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 fix > >> 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=com.mongodb.ReadPreference$PrimaryReadPreference” 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=primaryPreferred" > would > >> read much easier than > >> > >> > "readPreference=com.mongodb.TaggableReadPreference$PrimaryPreferredReadPreference" > >> 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-component-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-component-tp5750234p5750247.html > Sent from the Camel Development mailing list archive at Nabble.com. >
