lucene-java-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael-O <1983-01...@gmx.net>
Subject Re: Excessive use of IOException without proper documentation
Date Sun, 04 Nov 2012 20:46:24 GMT
Shalom Shai,

Am 2012-11-04 20:25, schrieb Shai Erera:
> Hey Mike,
>
> I'm not sure that I like the idea of throwing LuceneException or
> SearchException everywhere. I've been there (long time ago) and I always
> hated it.

Not everywhere but in the top-level API would be sufficient. Some code 
is never used/should be directly by the client. What is the reason for 
why you dislike such an approach?

> First, what's the difference between 'new SearchException("Failed to read
> the index", ioe)' and 'new IOException("Failed to read the index",
> anotherIOE/anyE)'? Both don't give you any real way of detecting the
> problem in your code, maybe only in the logs.
>
> Perhaps you would like us to throw better IOE, i.e. rather than re-throwing
> low-level IO exceptions, wrap them w/ another IOE w/ a detailed message.
> That I can support, but do you have a concrete case where the exception
> that was thrown was not descriptive enough?

There is a tremendous difference. I have possibility at compile time to 
detect whether an IOE came from Lucene or some other IO related action.

Consider this code:

try {
File mike = new File(...);
Reader r = new InputStreamReader(new FIS(mike)):
searcher.search(...);
}
catch (IOE e) {
}

Now, how am I supposed to figure out at compile time where this IOE came 
from? I cannot neither react properly nor present a suitable user 
response unless I do create for every IOE piece a separate try-catch-block.
This is just bloat. I cannot even log correctly because I do not know 
what the source of the IOE was.

What am I excepted to do when every method throws just new Exception()?
Absolutely nothing they would all look alike at compile time.

> For instance, there's that "read past EOF" exception which is annoying b/c
> you can tell by the stacktrace where it happened, but not necessarily why
> it happened. However, these are fairly low in our call stack, and I doubt
> that even Lucene code itself can give meaningful messages to all low-level
> IOEs. Yet, the exception does tell you that's usually an index corruption
> case, or some other bug ... when Lucene detects the index is corrupt, it
> throws the explicit CorruptIndexEx, otherwise, this must be inferred from
> the stacktrace. Not perfect, but throwing IndexingException won't improve
> it either.

It would cover at least the case described above.

> So I'm +1 for making sure our exceptions are as informative as they can
> get. I'm -1 for starting to throw LuceneException all over the place - to
> me it's like IOE. Worse, if we start throwing LuceneException, people might
> be tempted to wrap any Ex w/ LuceneEx, even ArrayIndexOutOfBound etc. I
> don't want that.

That should not be the case. IndexOutOfBoundsException is an unchecked 
exception.
There is a huge difference to IOE which is a checked exception. Joshua 
Bloch exactly describes the case of the IOOBE in item 58. IIOOBE must 
remain as unchecked because it indicates a violation of the API 
contract. E.g., a corrupt index is recoverable and not a programming 
error. This can happen through various reasons you cannot control.

> If you've hit exceptions which could use better messages, I prefer that we
> concentrate on them, rather than complicating the exceptions throwing
> mechanism.

I definitively will. As I always do with OSS.

> On Sun, Nov 4, 2012 at 7:25 PM, Michael-O <1983-01-06@gmx.net> wrote:
>
>> Hi Simon,
>>
>> there are generally two very good resources how exceptions should be
>> handled in Java. I will quote both:
>>
>> 1. Oracle's JavaDoc Style Guide: http://www.oracle.com/**
>> technetwork/java/javase/**documentation/index-137868.**html#throwstag<http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#throwstag>
>> 2. Joshua Bloch's book Effective Java, chapter 9:
>> http://my.safaribooksonline.**com/book/programming/java/**9780137150021<http://my.safaribooksonline.com/book/programming/java/9780137150021>
>>
>> Am 2012-11-02 22:27, schrieb Simon Willnauer:
>>
>>   Hey,
>>>
>>> On Fri, Nov 2, 2012 at 2:20 PM, Michael-O <1983-01-06@gmx.net>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>> why does virtually every method (exaggerating) throw an IOE? I know
>>>> there might be a failure in the underlying IO (corrupt files,
>>>> passing checked exc up, etc) but
>>>>
>>>> 1. Almost none of the has a JavaDoc on it
>>>>
>>>
>>> what should the javadoc say? I mean it would repeat itself all the
>>> time no?
>>>
>>
>> At least one would know why this one is thrown. See source one.
>>
>>
>>   2. Throwing an IOE from most of the methods doesn't really help.
>>>> You cannot create separate catch blocks. You have to rip your code
>>>> apart in tens of try catch blocks.
>>>>
>>>> Here is a simple example: Both IndexSearcher#search and
>>>> IndexSearcher#doc throw an IOE with any further documentation. I
>>>> don't have the chance to detect where the exception has happened
>>>> nor I can pass something reasonable back to the user. And both
>>>> methods are keypoints in Lucene.
>>>>
>>>
>>> can you elaborate what you would change to make this easier
>>> digestible for you? I mean specialized exceptions class would be a
>>> mess here really.
>>>
>>
>> Not only for me but for everyone.
>>
>> Generally, good exception handling should setup a simple exception
>> hierarchy, not more than three levels. Two base classes for checked and
>> unchecked exceptions. A main exception for a logical group like searching,
>> retreival, query checking, etc.
>>
>> LuceneException
>> |-- SearchException
>> |-- RetrievalException
>> |-- ...
>>
>> One should really wrap IOE in something like 'new SearchException("Failed
>> to read the index", ioe)'. I would at least know that this one came from
>> the index searcher.
>>
>> Really advisable are items 61, 62 and especially 63.
>>
>>
>>   E.g., there are exceptions thrown in IndexSearcher without any
>>>> message. Simply empty stubs.
>>>>
>>>
>>> I agree we should fix them. I already committed some fixes to
>>> IndexSearcher. Can you come up with a patch that fixes some more in
>>> core?
>>>
>>> running grep -R "new.*Exception()" * | wc -l
>>>
>>> yields 82 in core so there is room for improvement.
>>>
>>>
>>>> Lucene 4.0 received a complete API overhaul. Why was no action
>>>> taken to clean up exception management?
>>>>
>>>
>>> I'd really like to hear what you have in mind. can you elaborate?
>>>
>>
>> Continuing my answer from above. Have you ever worked with the Spring
>> Framework? They apply a very nice exception translation pattern. All
>> internal exceptions are turned to specialized unchecked exceptions like
>> AuthenticationExceptoin, DaoAccessException, etc.
>>
>> Lucene has already good exceptions like CorruptIndex, IndexNotFound,
>> LockObtainFailed, etc.
>>
>> Of course, such a step would break backwards-compat but this is possible
>> for 5.0 and this would require a general consensus of the devs improving
>> this issue.
>>
>> I think that such a great framework can do better to inform programmers
>> and users what has really failed especially if you design a public API.
>> We have succeeded exceptional results with the Lucene framework. I am
>> someone who likes giving help back to OSS projects.
>>
>> Hopefully, this is enough input for this message. We could discuss this
>> further is there is a real interest from the devs.
>>
>>
>> Mike
>>
>> ------------------------------**------------------------------**---------
>> To unsubscribe, e-mail: java-user-unsubscribe@lucene.**apache.org<java-user-unsubscribe@lucene.apache.org>
>> For additional commands, e-mail: java-user-help@lucene.apache.**org<java-user-help@lucene.apache.org>
>>
>>
>


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


Mime
View raw message