Roy,
Thanks for the review and comments. My comments inline below.
Roy Ward wrote:
(1) You only added timeouts to:
public TopDocs search(Weight weight, Filter filter, final int nDocs)
It's confusing if timeout functionality is not also added to:
public TopFieldDocs search(Weight weight, Filter filter, final int nDocs,
Sort sort)
Good catch. That was an oversight. The necessary changes were made to
TopFieldDocCollector.java, but you are right the changes to
public TopFieldDocs search(Weight weight, Filter filter, final int nDocs,
Sort sort)
were not in the patch. I have this fixed locally and will submit a
patch shortly.
(2) Estimating the the number of results is a good idea, however it breaks
some of the code in Hits.java when the Vector of results is not as long as
expected. This either needs more work or just returning the number or
results actually found. Perhaps a separate method for getting the estimate
in the case of partial results would be the way to go.
Is there a test case that shows this breakage, or can you point me to
the code in Hits.java that my patch causes problems with? Sorry, I'm
not seeing it.
(3) The timer, consisting of a whole lot of millisecond pauses (if the
resolution is 1) is not accurate (certainly under load). There needs to be
at least an occasional call to an accurate timer. It would also be better to
replace getCounter() by something like getMilliseconds() so the caller does
not need to know the resolution of the timer.
I wouldn't expect anyone to actually use a 1 ms resolution. That is in
the provided test case simply because it almost guarantees a timeout
occurs. The accuracy of the timeout as long as it is reasonably close
isn't terribly important. The typical use case as I see it would be to
preempt the occasional (< 1%) queries that take an unreasonable amount
of time to complete. For example the timer may be configured for 10
counts of 100 ms (1 second). If that isn't preempted until 1.1 seconds
have elapsed, I think my operations team will still be happy.
I do like your suggestion of getMilliseconds(). It is clearer. I've
made this change locally and will submit a patch shortly.
-Sean
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
