Also if there are going to be checks against this, if its really going to become the primary entry point for deciding the version of a segment and what exception to throw and so on, then I strongly disagree with string encoding.
In my opinion we already have such a scheme figured out (CodecUtil methods) with exception handling and everything... On Sun, Sep 14, 2014 at 7:58 AM, Robert Muir <[email protected]> wrote: > I agree, can we remove all the checks here? They are bogus in a few ways: > > 1. Version.parse claims its forwards compatible, but the ctor throws > these exceptions. > 2. These exceptions are thrown without indicating what the actual value was. > 3. These exceptions are thrown with no context (the toString of the > input file itself). > > So, I don't think Version should do these checks, at least, if it > wants to do them, it needs to step up to the plate and do them > correctly. > > On Sun, Sep 14, 2014 at 7:19 AM, Uwe Schindler <[email protected]> wrote: >> Especially, as I said in my previous email: ist in my opinion bad to throw >> IllegalArgumentException on version's parse if it is a vlaid version. >> >> I tried it locally and created an Index with version 6.0 written to disk (I >> hacked it). If I try top open it with IndexReader of Lucene 4, so it should >> throw IndexTooNewException! But in fact it did not, because Version.parse() >> failed earlier! So we are inconsistent with Exceptions! It should in fact >> really throw IndexFormatTooNewException! >> >> In my opinion, the bounds checks in the Version ctor should be "optional", >> so when parsing versions from index files we have a chance to throw the >> "correct exception". >> >> Uwe >> >> ----- >> Uwe Schindler >> H.-H.-Meier-Allee 63, D-28213 Bremen >> http://www.thetaphi.de >> eMail: [email protected] >> >> >>> -----Original Message----- >>> From: Michael McCandless [mailto:[email protected]] >>> Sent: Sunday, September 14, 2014 12:46 PM >>> To: Lucene/Solr dev >>> Subject: Re: svn commit: r1624773 - in /lucene/dev/branches/branch_4x: ./ >>> lucene/ lucene/core/ >>> lucene/core/src/java/org/apache/lucene/codecs/lucene46/Lucene46Segme >>> ntInfoWriter.java >>> >>> On Sat, Sep 13, 2014 at 10:23 PM, Ryan Ernst <[email protected]> wrote: >>> > How would the Version be constructed with an invalid major version, >>> > given this exact check in the constructor (and the fact that the only >>> > way to construct is through Version.parse)? >>> >>> I think it makes sense to be defensive here and have the check in two >>> places. >>> >>> This also guards against any future changes to Version that somehow allow >>> this ... it sure look like Version can't ever be created in an "out of >>> bounds" >>> state today, but who knows tomorrow ... >>> >>> Mike McCandless >>> >>> http://blog.mikemccandless.com >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: [email protected] For additional >>> commands, e-mail: [email protected] >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
