camel-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Babak Vahdat <babak.vah...@swissonline.ch>
Subject Re: About a bug of the camel-mongodb component...
Date Tue, 15 Apr 2014 21:13:43 GMT
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,
> 
> 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
> &lt;

> babak.vahdat@

> &gt;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
>> > &lt;
>>
>> > babak.vahdat@
>>
>> > &gt;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.
>>





--
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.

Mime
View raw message