I'll open an issue and work out a patch.
Basically it means infoStream != null, although in LogMergePolicy I might
add a specific method for that, because the messages are output if the
IndexWriter member is not null and its infoStream is not null (this check is
done by IndexWriter).
Therefore I think I'll add a method to IndexWriter messagesEnabled() which
returns true if the infoStream is not null for use by other classes (rather
than the implicit iw.getInfoStream() != null). BTW, getInfoStream() is not
called by any class in Lucene, except one test class.
What do you think about adding this method, and its name?
On Fri, Dec 5, 2008 at 3:35 PM, Michael McCandless <
[EMAIL PROTECTED]> wrote:
>
> I agree, it is a best practice and we should follow it. Can you work out a
> patch & open an issue? I assume this means "if (infoStream != null)..." in
> this case.
>
> Mike
>
>
> Shai Erera wrote:
>
> Hi
>>
>> As I looked at the code in LogMergePolicy (and its sub-classes), I came
>> across such lines:
>>
>> message("findMergesToExpungeDeletes: " + numSegments + " segments");
>>
>> Those lines print to the info stream (eventually) if it's not null.
>>
>> If one follows Java logging best practices, then any logging message
>> should look similar to this:
>> if (logger.isLoggable(Level)) {
>> logger.log(Level, msg);
>> }
>>
>> The reason is that when logging messages, one usually does not pay
>> attention to any performance issues, like String concatenation. Therefore,
>> checking if logging is enabled saves building the String just to discover
>> later that logging is not enabled.
>>
>> I haven't checked other places in the code, because I'd like to get the
>> committers opinion on this first. Imo, those strings are created
>> unnecessarily (the above message creates 5 strings) since most of the time
>> the info stream is null.
>>
>> I can provide a patch to fix it by first checking if logging (or whatever
>> other name you'd like to give it) is enabled before attempting to output any
>> message. The LogMergePolicy classes are one example that I've run at, but
>> I'm sure there are other places in the code.
>>
>> I don't foresee any great performance improvements by this fix, except
>> just following best practices.
>>
>> What do you think?
>>
>> Shai
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
