I think one reason to keep it there is Codec.availableCodecs(). If a test relies on it to pick a Codec at random, he won't be able to choose RWCodec.
Sorry if I go back and forth, I'm just trying to understand these dependencies. This boolean seems like an easy solution, I don't mind adding it in LUCENE-5189. Shai On Sat, Aug 31, 2013 at 6:34 AM, Robert Muir <[email protected]> wrote: > We do not need to do anything complicated here. > > we just need to add back the boolean (impersonation is active) like we > did for 3.x back compat. > > the test that rely upon this (currently there are zero of them) can > unset it appropriately. > > > On Fri, Aug 30, 2013 at 11:24 PM, Shai Erera <[email protected]> wrote: > > I tried that and of course it didn't work, but because of lines like > these > > in TestRuleSetupAndRestoreClass: > > > > codec = Codec.forName("Lucene40"); > > assert codec instanceof Lucene40RWCodec : "fix your classpath to > have > > tests-framework.jar before lucene-core.jar"; > > assert (PostingsFormat.forName("Lucene40") instanceof > > Lucene40RWPostingsFormat) : "fix your classpath to have > tests-framework.jar > > before lucene-core.jar"; > > > > I understand the third line, but why do we need to do Codec.forName here? > > I searched for Codec.forName and there are 14 matches, some of which are > in > > tests which should just do e.g. "new SimpleTextCodec()" or "new > > Lucene40RWCodec()". What do you think? > > > > Shai > > > > > > On Sat, Aug 31, 2013 at 6:13 AM, Shai Erera <[email protected]> wrote: > >> > >> On LUCENE-5189 I ran into the following problem: I wrote a test which > >> creates an index with "old" unsupported Codecs (40, 41, 42) and then > tries > >> to update them, expecting to fail since those Codecs don't support > >> numeric-dv-updates, as well as their Consumer isn't even shipped and > they > >> throw UOE in fieldsConsumer(). But, the test doesn't hit that exception, > >> because the RWCodecs are listed under test-framework/META-INF/services > and > >> are used instead of the real 40/41/42Codec. > >> > >> In light of what was discussed here, why do we need those Codecs listed > in > >> services? > >> > >> Shai > >> > >> > >> On Fri, Aug 30, 2013 at 5:28 PM, Shai Erera <[email protected]> wrote: > >>> > >>> I removed it from META-INF and all tests pass. So I guess it's safe to > >>> remove it completely. > >>> > >>> > >>>> Actually Facet45Codec is not needed at all. I dont understand why we > >>>> have a codec class here at all. > >>> > >>> > >>> Because it takes FacetIndexingParams and returns FacetDVF for all > fields > >>> that appear in the indexing params. Can you do that without a Codec > class? > >>> It's also easier to use than users figuring they should extend > >>> Lucene45Codec to plug-in FacetDVF. > >>> > >>> I'll just remove it from META-INF? > >>> > >>> Shai > >>> > >>> > >>> On Fri, Aug 30, 2013 at 5:16 PM, Robert Muir <[email protected]> wrote: > >>>> > >>>> On Fri, Aug 30, 2013 at 10:01 AM, Shai Erera <[email protected]> > wrote: > >>>> > I don't think we should go that far. If you extend Lucene45Codec you > >>>> > basically agree to the entire index format, but are given a chance > to > >>>> > control per-field postings and doc values. Otherwise, make your own > >>>> > Codec, > >>>> > and then you'll need to register it in META-INF/services. > >>>> > > >>>> > The assert I proposed to make in the ctor is only for "education > >>>> > purposes" > >>>> > -- apps need not register their Lucene45CodecExtension in services. > We > >>>> > can > >>>> > document it, and assertions would help verify it. > >>>> > > >>>> > >>>> I don't think we need anything here really: Facet45Codec shouldnt be > >>>> in META-INF. > >>>> > >>>> Actually Facet45Codec is not needed at all. I dont understand why we > >>>> have a codec class here at all. > >>>> > >>>> --------------------------------------------------------------------- > >>>> 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] > >
