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 Wed, 16 Apr 2014 07:14:28 GMT
Hi Ràul,

Now the other ticket (CAMEL-7370) has been fixed as well.

And regarding the reason why I went for ReadPreference#valueOf() instead of
ReadPreference#[primary/secondary/nearest/etc.] (that's the +0.5 points you
gave about it):

- The logic to be implemented in this way is much easier, as it's one single
line of code compared to other option which one would need to do if/elseif
etc.
- If the feature versions of MongoDB-Java-Driver provides more read
preference options we would NOT need to change anything on our side. However
using ReadPreference#[primary/secondary/nearest/etc.] would require one more
"else if" block to be added while upgrading the driver ;)

Thanks again for your comments on this thread!
Babak


Babak Vahdat wrote
> 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-tp5750234p5750280.html
Sent from the Camel Development mailing list archive at Nabble.com.

Mime
View raw message