Analysis Consumers should end and close any TokenStreams [LUCENE-3456]

[asfimport]

While converting consumers over to using reusableTokenStream, I notice many don't call end() or close() on TokenStreams.

Even if they are test TSs only created once, we should follow the defined usage pattern.

---

Migrated from LUCENE-3456 by Chris Male, updated Jan 30 2013
Parent: #4469
Attachments: LUCENE-3456.patch (versions: 2)

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I have some idea how to assert this is tests (now that Analyzer.(reuseable)tokenStream is final everywhere):
We can wrap the TokenStream reaturned by those two methods in Analyzer using a AssertingTokenFilter that simply tracks in its methods the correct usage of reset() [throw AssertionFailed if missing], increment(), end(), close() [this last one is hard to track].

The idea is:
If assertions are enabled, the tokenstream-returning methods in Analyzer.java should check the desiredAssertionStatus of this' class and wrap the TokenStream using that TokenFilter described before. If a consumer then forgets to call any of these methods oir does this in wrong order, an AssertionError is thrown.

I could hack something together (yes, it is a hack).

[asfimport]

Chris Male (migrated from JIRA)

How would you track the end and close invocations? (or lack thereof)?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I could hack something together (yes, it is a hack).

We already do this in MockTokenizer?

The problem is that many disable the assertions (MockTokenizer.setEnableChecks)

So, I dont think we need a new tokenstream to test this...

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Yes it's down low, my idea was to do it top-level. But you are right, it's not really needed. And we cannot 100% handle end/close unless we reuse.

The problem my approach would solve is that Solr would be asserted all the time, not only if it uses Mock*, which is rarely does?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

The problem my approach would solve is that Solr would be asserted all the time, not only if it uses Mock*, which is rarely does?

But perhaps a better solution to this problem would be to use MockTokenizer etc in Solr test configs (there are already enough tests testing the example config)

[asfimport]

Chris Male (migrated from JIRA)

Sorting this out for the current codebase isn't a huge challenge. I work from incrementToken() / reuseable/TokenStream() calls, and see whats being called in their vicinity. But going forward, I agree using Mock* in Solr more makes sense.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

here is a patch cutting over solr/core...

this looks to have uncovered some bugs, i didnt fix any of them.

additionally i think we should do the same thing for solrj, contrib/*, etc.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I'm iterating on this patch a bit, so we don't do duplicate work.

it probably won't find all the problems, but it will fix some.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

attached is a patch cutting over all uses of whitespace->mocktokenizer in solr test configs, and fixing the errors this found.

i'd like to commit shortly, we can improve the coverage more by looking at other configs and cutting over things like keywordtokenizer

[asfimport]

Chris Male (migrated from JIRA)

Where are we at with this Robert? I'm about to complete converting over to reusableTokenStream() in Solr. Should I work on that or wait for you to finish here?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

i have a new patch i made on the airplane, but it found more bugs... ill fix them tonight and commit!

[asfimport]

Chris Male (migrated from JIRA)

Alright I'll stick to Lucene and modules for a bit.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

ok, i committed my stuff, just test bugs (ill backport though)... go ahead!

[asfimport]

Steven Rowe (@sarowe) (migrated from JIRA)

Robert, Chris, Uwe: Can this be resolved now?