lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Shai Erera (JIRA)" <>
Subject [jira] [Commented] (LUCENE-3786) Create SearcherTaxoManager
Date Wed, 10 Apr 2013 03:48:16 GMT


Shai Erera commented on LUCENE-3786:

Few comments:

* This assert in the test {{assertEquals(1, results.size());}} is kinda moot because we always
return a FacetResult, even if empty. Perhaps you can assert that if the acquired reader.maxDoc
is > 0, the returned FacetResult.rootNode has at least one child with count that is at
least 1?

* Maybe change the end of the test to a single-line IOUtils.close()?

* You wrote previously that the test uses LineFileDocs, but I don't see it. It seems it only
adds facets to documents? If so, can it go back to newDirectory()?

* It's good that you identify replaceTaxonomy, makes the code safer.

* TR.getTaxoEpoch: maybe instead of adding it to TR you can use the one on DTW (make it public,
@lucene.internal)? It's odd that it documents that this epoch is returned only for an NRT
TR, because the epoch is recorded on the taxo index commit data, so conceptually there's no
reason why it shouldn't always return it. Yet, since this epoch is used internally, between
TW and TR, I prefer not to expose it too much. Hmmm, but then you may hit a false positive
where the returned TR is valid, yet just in between the checks the app called replaceTaxo.
But I think that's ok since it means the check will fail on the next refresh attempt. Really,
if ever DTW.epoch changes, we should fail.

* I don't know how important it is, but perhaps given the short discussion we had above, it
would be good to add a 1-liner to decRef why the method seems unprotected, but in reality
it's the best we can do?

* In refreshIfNeeded, I understand this code {{newReader.decRef()}} is equivalent to closing
{{newReader}} (if epoch has changed). But after I received a question yesterday from a someone
who did not understand why we don't call close(), perhaps we should, for clarity?

Otherwise this looks great! When I worked on it in the past, DTR wasn't NRT and the sync was
a nightmare. Making it NRT really simplified this manager!
> Create SearcherTaxoManager
> --------------------------
>                 Key: LUCENE-3786
>                 URL:
>             Project: Lucene - Core
>          Issue Type: New Feature
>          Components: modules/facet
>            Reporter: Shai Erera
>            Assignee: Michael McCandless
>            Priority: Minor
>             Fix For: 5.0, 4.3
>         Attachments: LUCENE-3786-3x-nocommit.patch, LUCENE-3786.patch, LUCENE-3786.patch
> If an application wants to use an IndexSearcher and TaxonomyReader in a SearcherManager-like
fashion, it cannot use a separate SearcherManager, and say a TaxonomyReaderManager, because
the IndexSearcher and TaxoReader instances need to be in sync. That is, the IS-TR pair must
match, or otherwise the category ordinals that are encoded in the search index might not match
the ones in the taxonomy index.
> This can happen if someone reopens the IndexSearcher's IndexReader, but does not refresh
the TaxonomyReader, and the category ordinals that exist in the reopened IndexReader are not
yet visible to the TaxonomyReader instance.
> I'd like to create a SearcherTaxoManager (which is a ReferenceManager) which manages
an IndexSearcher and TaxonomyReader pair. Then an application will call:
> {code}
> SearcherTaxoPair pair = manager.acquire();
> try {
>   IndexSearcher searcher = pair.searcher;
>   TaxonomyReader taxoReader = pair.taxoReader;
>   // do something with them
> } finally {
>   manager.release(pair);
>   pair = null;
> }
> {code}

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message