[
https://issues.apache.org/jira/browse/LUCENE-3786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13627435#comment-13627435
]
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: https://issues.apache.org/jira/browse/LUCENE-3786
> 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: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
