MergePolicy and IndexWriter methods argument

[Shai Erera <se...@gmail.com>]

Hi

While reading LogMergePolicy I noticed that it uses IndexWriter's member and
method arg inconsistently:
1) Some methods that receive IW as a parameer, do: this.indexWriter =
indexWriter, and then use the member instance.
2) Others set the member instance, but continue to use the method arg.
3) Others don't set the member instance at all.
4) Some use the member, w/ the possibility of hitting NPE (if, say, the
findMerge* methods were not called yet).

As far as I understand, the member instance is defined just for methods that
need to use IW, but since the class does not require IW to be passed during
construction, they rely on one of the findMerge* methods to set the member
instance to the one they got. Is that right? I guess it is possible for the
same MergePolicy instance to receive different IW instances during its life
span, but is it something we should support?

Leaving back-compat aside for a moment, if a MP lives within an IndexWriter,
why not require an IW instance to be passed during an MP construction
(passing 'this' for IW own instantiation)? Then we can remove the IW method
arg and rely, safely, on the existence of IW.

Shai

Re: MergePolicy and IndexWriter methods argument

[Shai Erera <se...@gmail.com>]

I'll open an issue and work out a patch. Though this deprecation stuff is
what I was worried of - they always tend to expand more than I plan to :).

Shai

On Sun, Jul 26, 2009 at 9:44 PM, Michael McCandless <
lucene@mikemccandless.com> wrote:

> I agree it's messy now.  I think requiring the writer to be specified
> on creating the merge policy would make sense.  You can't safely share
> a LMP today across multiple writers, yet the class "pretends" that you
> can...
>
> You'd also need to deprecate the public methods that take a writer in
> favor of new methods that don't take one (and use the member instead)?
>
> Wanna cons up a patch?
>
> Mike
>
> On Sun, Jul 26, 2009 at 7:30 AM, Shai Erera<se...@gmail.com> wrote:
> > Hi
> >
> > While reading LogMergePolicy I noticed that it uses IndexWriter's member
> and
> > method arg inconsistently:
> > 1) Some methods that receive IW as a parameer, do: this.indexWriter =
> > indexWriter, and then use the member instance.
> > 2) Others set the member instance, but continue to use the method arg.
> > 3) Others don't set the member instance at all.
> > 4) Some use the member, w/ the possibility of hitting NPE (if, say, the
> > findMerge* methods were not called yet).
> >
> > As far as I understand, the member instance is defined just for methods
> that
> > need to use IW, but since the class does not require IW to be passed
> during
> > construction, they rely on one of the findMerge* methods to set the
> member
> > instance to the one they got. Is that right? I guess it is possible for
> the
> > same MergePolicy instance to receive different IW instances during its
> life
> > span, but is it something we should support?
> >
> > Leaving back-compat aside for a moment, if a MP lives within an
> IndexWriter,
> > why not require an IW instance to be passed during an MP construction
> > (passing 'this' for IW own instantiation)? Then we can remove the IW
> method
> > arg and rely, safely, on the existence of IW.
> >
> > Shai
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>

Re: MergePolicy and IndexWriter methods argument

[Michael McCandless <lu...@mikemccandless.com>]

I agree it's messy now.  I think requiring the writer to be specified
on creating the merge policy would make sense.  You can't safely share
a LMP today across multiple writers, yet the class "pretends" that you
can...

You'd also need to deprecate the public methods that take a writer in
favor of new methods that don't take one (and use the member instead)?

Wanna cons up a patch?

Mike

On Sun, Jul 26, 2009 at 7:30 AM, Shai Erera<se...@gmail.com> wrote:
> Hi
>
> While reading LogMergePolicy I noticed that it uses IndexWriter's member and
> method arg inconsistently:
> 1) Some methods that receive IW as a parameer, do: this.indexWriter =
> indexWriter, and then use the member instance.
> 2) Others set the member instance, but continue to use the method arg.
> 3) Others don't set the member instance at all.
> 4) Some use the member, w/ the possibility of hitting NPE (if, say, the
> findMerge* methods were not called yet).
>
> As far as I understand, the member instance is defined just for methods that
> need to use IW, but since the class does not require IW to be passed during
> construction, they rely on one of the findMerge* methods to set the member
> instance to the one they got. Is that right? I guess it is possible for the
> same MergePolicy instance to receive different IW instances during its life
> span, but is it something we should support?
>
> Leaving back-compat aside for a moment, if a MP lives within an IndexWriter,
> why not require an IW instance to be passed during an MP construction
> (passing 'this' for IW own instantiation)? Then we can remove the IW method
> arg and rely, safely, on the existence of IW.
>
> Shai
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org