lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Wolf Siberski <siber...@l3s.de>
Subject Re: patch - DEFAULT_ vars in IndexWriter non-final and DEFAULT for useCompoundFile
Date Fri, 25 Feb 2005 11:45:59 GMT
Kevin A. Burton wrote:
> 
> Yet another patch.
> 
> Right now there are a number of DEFAULT_ vars in IndexWriter.  On init 
> these are fetched from system properties and initialized.
> 
> One problem here though is that once the VM has been started there's no 
> way to chnage them.  I'd prefer to set these via my own initializer as 
> we use a reflection system to set system properies and this will tie 
> into our system much better.  It would also allow someone to write their 
> own initializer to just set 
> org.apache.lucene.index.IndexWriter.DEFAULT_MERGE_FACTOR = .... or do 
> this dynamically via code.  (not everyone has access to system properties)

I see following issues with your patch:
- you changed the DEFAULT_... semantics from constant to modifiable,
   but didn't adjust the names according to Java conventions (default_...).
- it seems even you don't need to have them non-constant, you just
   want to fetch the default values from another source. This is an
   indication that your approach isn't really the appropriate solution
   to your problem.
- you can achieve the same by writing your own IndexWriterFactory which
   sets the corresponding values after creating a new IndexWriter.
   Should be ~30 lines of code. It only makes sense to include a
   patch if either a solution is impossible with the current code
   or *a lot* of (potential) users have to work around something.

> Of course now let me pre-emptively respond to your inevitable replies:
> 
> 1. We can't allow the values to change at runtime.
> 
> Why not?  I can set them via instance variables at anytime.
> 
> 2. These are supposed to be constant variables.
> 
> They don't need to be.  Keep an open mind! ;)

This is (IMHO) the wrong line of reasoning.

In general, an API should be as small and restrictive as possible,
because that leaves developers more freedom to evolve the system.

Therefore to extend the API, you need a better argument than
'it will not break the current implementation'. The only justification
could be that users are needing the extension to accomplish a specific
task. In your case it doesn't seem so (see above).

So, if anything at all, I would rather opt for making these constants
private :-).

> Another note..  Why are we obfuscating the fully qualified var name?
> 
> Instead of users setting a property of 'org.apache.lucene.mergeFactor" 
> why not just have them set "     
> "org.apache.lucene.index.IndexWriter.DEFAULT_MERGE_FACTOR"
> 
This is typically a bad idea, because it ties configuration files
to a specific implementation. For example, suppose we want to
move these constants to a new configuration class.
Then we could either adapt configuration properties to the new
attribute location, thus breaking all existing configuration files
(very bad). Or we could stay with the previous name, which would
be very misleading (also pretty bad).
Therefore, it is much better using configuration property names
which are informative, but independent of specific code.

--Wolf

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


Mime
View raw message