Re: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

Robert Muir
Hi Martinj: thanks for looking into this!

I think have a better fix for these:

the problem is actually in the AssertingAtomicReaders that
AssertingDirectoryReader wraps its subreaders with.
So I added the invisible-cache-key hack there, and removed it
completely from LuceneTestCase.

I tested this with the hudson seeds that failed (at their appropriate
revisions) and it seems to work fine.
I also ran tests for queries/grouping/join with -Dnightly=true,
-Dtests.multiplier=5, etc etc a few times and it all works.

I'd really like to have AssertingDirectoryReader being used again.
If there are problems we can just back out the change.

On Thu, Jul 19, 2012 at 5:48 AM,  <[hidden email]> wrote:

> Author: mvg
> Date: Thu Jul 19 09:48:04 2012
> New Revision: 1363272
>
> URL: http://svn.apache.org/viewvc?rev=1363272&view=rev
> Log:
> Fix of rare FC insanity during tests that have occurred in grouping & joining tests.
>
> Modified:
>     lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java
>
> Modified: lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java
> URL: http://svn.apache.org/viewvc/lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java?rev=1363272&r1=1363271&r2=1363272&view=diff
> ==============================================================================
> --- lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java (original)
> +++ lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java Thu Jul 19 09:48:04 2012
> @@ -1048,7 +1048,7 @@ public abstract class LuceneTestCase ext
>              if (r instanceof AtomicReader) {
>                r = new FCInvisibleMultiReader(new AssertingAtomicReader((AtomicReader)r));
>              } else if (r instanceof DirectoryReader) {
> -              r = new FCInvisibleMultiReader(new AssertingDirectoryReader((DirectoryReader)r));
> +              r = new FCInvisibleMultiReader((DirectoryReader)r);
>              }
>              break;
>            default:
>
>



--
lucidimagination.com

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

Martijn v Groningen-2
Thanks for also taking a look at the test failures!

On 20 July 2012 19:14, Robert Muir <[hidden email]> wrote:
> Hi Martinj: thanks for looking into this!
>
> I think have a better fix for these:
>
> the problem is actually in the AssertingAtomicReaders that
> AssertingDirectoryReader wraps its subreaders with.
> So I added the invisible-cache-key hack there, and removed it
> completely from LuceneTestCase.
Yes this was the problem. I think this fix wouldn't make hudson fail
again. Since the
FC insanity can't be detected.

During some test runs the AtomicReaderContext (in
Collector#setNextReader method) contains a
SlowCompositeReaderWrapper and this reader wraps another reader
something like this:
SlowCompositeReaderWrapper(ParallelCompositeReader(FCInvisibleMultiReader(StandardDirectoryReader(segments_4:9
_0(5.0):C463 _1(5.0):C930 _2(5.0):C378 _3(5.0):C184))))

This used to cause the FC insanity test failures, but is this a
scenario that actually needs to be tested? Having a top level
readercontext in the setNextReader method seems like wrong usage to
me.

Martijn

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

Uwe Schindler
Hi,

> Thanks for also taking a look at the test failures!
>
> On 20 July 2012 19:14, Robert Muir <[hidden email]> wrote:
> > Hi Martinj: thanks for looking into this!
> >
> > I think have a better fix for these:
> >
> > the problem is actually in the AssertingAtomicReaders that
> > AssertingDirectoryReader wraps its subreaders with.
> > So I added the invisible-cache-key hack there, and removed it
> > completely from LuceneTestCase.
> Yes this was the problem. I think this fix wouldn't make hudson fail again. Since
> the FC insanity can't be detected.
>
> During some test runs the AtomicReaderContext (in Collector#setNextReader
> method) contains a SlowCompositeReaderWrapper and this reader wraps
> another reader something like this:
> SlowCompositeReaderWrapper(ParallelCompositeReader(FCInvisibleMultiRead
> er(StandardDirectoryReader(segments_4:9
> _0(5.0):C463 _1(5.0):C930 _2(5.0):C378 _3(5.0):C184))))
>
> This used to cause the FC insanity test failures, but is this a scenario that
> actually needs to be tested? Having a top level readercontext in the
> setNextReader method seems like wrong usage to me.

That is wanted! We want to test SlowMultiReaderWrapper and verify that it really behaves like a full conformant AtomicReader. Because of that we sometimes war with it. The fix Robert did is fine and is identical to the one we did in the past. As this is no real Lucene usage pattern, not detecting the obvious cache violation is wanted here.

For incorrect Lucene usage (like getting a field cache entry on a slow wrapper), we can still warn the user. Buf for tests this insanity is wanted by the wrapping.

Uwe


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

Martijn v Groningen-2
> That is wanted! We want to test SlowMultiReaderWrapper and verify that it really behaves like a full conformant AtomicReader. Because of that we sometimes war with it. The fix Robert did is fine and is identical to the one we did in the past. As this is no real Lucene usage pattern, not detecting the obvious cache violation is wanted here.
Ok. I was just wondering about the fact that a toplevel readercontext
is given as argument to Collector#setNextReader(). It confused me, but
as you said it is no real Lucene usage pattern.

>
> For incorrect Lucene usage (like getting a field cache entry on a slow wrapper), we can still warn the user. Buf for tests this insanity is wanted by the wrapping.
I think the SlowCompositeReaderWrapper jdocs warns the user already
enough, so that is fine for now.

Martijn

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

Uwe Schindler
Hi,

If you use a plain SlowCompositeReaderWrapper without an FCInvisible wrapper on the MultiReader behind, the FC insanity checker will warn the user! So
FieldCache.DEFAULT.getInts(SlowCompositeReaderWrapper.wrap(directoryReader), field) will create insanity and the detector will find that. The reason for this is, because SlowCompositeReader returns the cache key of the directoryReader it wraps, so the insanity checker sees the child readers and shout "ALARM!". We just "hide" the composite reader key, so the random wrapping causes no insanity, because the atomic reader wrapper looks really atomic to the insanity checker.

Uwe

-----
Uwe Schindler
H.-H.-Meier-Allee 63, D-28213 Bremen
http://www.thetaphi.de
eMail: [hidden email]


> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On
> Behalf Of Martijn v Groningen
> Sent: Monday, July 23, 2012 10:44 AM
> To: [hidden email]
> Subject: Re: svn commit: r1363272 - /lucene/dev/trunk/lucene/test-
> framework/src/java/org/apache/lucene/util/LuceneTestCase.java
>
> > That is wanted! We want to test SlowMultiReaderWrapper and verify that it
> really behaves like a full conformant AtomicReader. Because of that we
> sometimes war with it. The fix Robert did is fine and is identical to the one we
> did in the past. As this is no real Lucene usage pattern, not detecting the
> obvious cache violation is wanted here.
> Ok. I was just wondering about the fact that a toplevel readercontext is given as
> argument to Collector#setNextReader(). It confused me, but as you said it is no
> real Lucene usage pattern.
>
> >
> > For incorrect Lucene usage (like getting a field cache entry on a slow
> wrapper), we can still warn the user. Buf for tests this insanity is wanted by the
> wrapping.
> I think the SlowCompositeReaderWrapper jdocs warns the user already enough,
> so that is fine for now.
>
> Martijn
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email] For additional
> commands, e-mail: [hidden email]


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]