[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501966#comment-13501966 ] Gilad Barkai commented on LUCENE-3441: -- Patch looks very good. +1. > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch, LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501141#comment-13501141 ] Shai Erera commented on LUCENE-3441: bq. would the old TR ever receive a request to look up an out-of-bounds ord In fact, it could. Like IR, the old TR still lives for old queries in flight. It is possible that TR-1 received a FacetRequest with a category that it doesn't recognize, but because the cache is now shared, and the TR-2 recognizes that category and put it in the cache, TR-1 will receive its ordinal, which is out-of-bound. Took a re-look at the code, and I think that it's solvable by having TR do a bounds check. It is a very simple and cheap check, only one 'if', which will happen, in practice, only for the top-K categories and the FacetRequest initially. I.e., the entire faceted search process -- aggregating the categories, determining top-K etc., all happen on the ordinals that are known to the search index (IR-1, that is coupled with TR-1). In fact, the labeling that IR-1/TR-1 will do to the top-K cannot out-of-bound, so actually only the FacetRequest may out-of-bound. Still, I think that it's safer to add the bounds check to both caches. I will post a new patch soon, compiled with the comments from Sivan (thanks !), and removing the nocommit. > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501120#comment-13501120 ] Michael McCandless commented on LUCENE-3441: bq. Since the usage of openIfChanged is well defined, and assuming that people who interact with the taxonomy are aware Lucen's NRT, should we rely on the old taxo reader instances to not be used after openIfChanged? Hmm, for NRT the old reader should continue working just fine even if a new reader was openIfChange'd off of it. EG there may be queries in-flight still against the old reader. However, for TR, would the old TR ever receive a request to look up an out-of-bounds ord? It shouldn't, right? Can we declare that the results when you pass out of bounds ords are not defined, ie that's abuse of the API? > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13500953#comment-13500953 ] Sivan Yogev commented on LUCENE-3441: - Went through the patch and it looks good. One symmetry question I have - TR is now abstract class and TW still interface. Can it be that we should make TW also abstract class? A few minor comments: ParentArray: * constructors are public but have no Javadoc. Are they meant for external (outside the taxonomy package) usage? DTR: * Semi-cosmetic issue - I would move the first (non-public) constructor after the two public ones, confused me a bit to see that one first. * Line 127: unnecessary "to" in Javadoc of param directory. * 4000 appears 4 times in the code, would rather have it once as a constant. > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13500306#comment-13500306 ] Shai Erera commented on LUCENE-3441: The side effect of sharing the cache is for TR-1 to receive an ordinal which doesn't actually exist. We can document that if you use openIfChanged, but continue to use the old reader, then this sort of thing can happen. Really, this is not how people should use openIfChanged ... Another idea I had is to wrap the cache with an object that keeps track of 'upto'. I.e. TR-1's instance's upto will be e.g. 123. On openIfChanged we allocate a new instance for TR-2, with upto=1035, sharing the same LRU instance. On each access to the cache, we check if the returned ordinal is <= upto, otherwise return INVALID. But that adds another 'if' to every cache-check. How expensive is it, not sure yet, since the taxonomy is not accessed for so many facets. It feels like we need to make some compromise here - either at runtime performance or possible correctness. Since the usage of openIfChanged is well defined, and assuming that people who interact with the taxonomy are aware Lucen's NRT, should we rely on the old taxo reader instances to not be used after openIfChanged? > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13500263#comment-13500263 ] Shai Erera commented on LUCENE-3441: bq. why not have a single instance of the LRUCache for all time, and never call .clear() on it? That will help as long as previous TR instances are indeed on their way to die. Otherwise, if e.g. an app, for some reason, reopens a TR but doesn't close the old one and uses both (again, for some really unknown reason), then two TR instances might affect each other. Now, since that's a very stupid thing to do, I'm not sure that I care about this much, as long as we preserve correctness. Meaning, that that one instance may reduce the size of the cache, while another increases it - that's the app problem. That that the two instances might evict entries from the LRU cache left and center, that's the app problem. The correctness issues that I'm worried about is (suppose that TR-1 and TR-2 share the same instance): * TR-1 looks for a category "foo", doesn't find it and adds to the cache the fact that the category is unknown * TR-2 looks for the category "foo", which exists in its newer version of the taxonomy, and receives the ordinal -1, which denotes that the category doesn't exist --- WRONG !! To solve that, we could not store the fact that a category does not exist in the cache. Really, this shouldn't happen - apps do not ask the taxonomy for random categories. So then: * TR-1 looks for a category "foo", doesn't find it in the cache and DOES NOT update the cache w/ that info. It goes to disk, doesn't find it there, returns -1. * TR-2 looks for the category "foo", which exists in its newer version of the taxonomy, fetches it from disk and stores the ordinal in the cache. * TR-1 looks for the category "foo" again, now receives an ordinal which is larger than its taxonomy size --- might be a problem !! In general, since I don't think that apps access the taxonomy for random ordinals or categories, the second solution might be good. Never store in the cache the fact that an ordinal/category is not found + don't clear() the cache, only nullify its reference + hope for the best :)? > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13500239#comment-13500239 ] Michael McCandless commented on LUCENE-3441: I don't fully follow all changes in the patch, but generally this sounds like an awesome change! On that nocommit ... why not have a single instance of the LRUCache for all time, and never call .clear() on it? I suspect in practice that .clear() isn't helping much? Ie it's rare that an app would close the LTR and hold onto the reference to it ...? > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13500219#comment-13500219 ] Shai Erera commented on LUCENE-3441: bq. While I could just share the instance Sorry, forgot to complete the idea -- simply sharing the instances currently won't work, b/c e.g. when TR.close() is called, the cache is clear()'ed, which will affect the other instance as well. A simple solution would be to remove the 'final' modifier from the cache member, and nullify the cache on close(), instead of clear()-ing. That way, if we rely on TR instances that are reopened to be immediately closed by the caller, and sharing the cache instances may not be so bad. > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Core > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Assignee: Shai Erera >Priority: Minor > Attachments: LUCENE-3441.patch > > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[
https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13109735#comment-13109735
]
Shai Erera commented on LUCENE-3441:
bq. Shouldn't you also commit in the constructor
LuceneTaxonomyWriter behaves just like IndexWriter. Today (I think since 3.1),
opening an IndexWriter is just another transaction that you should commit if
you want IndexReaders to see it. So if you try:
{code}
IndexWriter w = new IndexWriter(emptyDir, new IWC());
IndexReader r = IndexReader.open(emptyDir);
{code}
you'll get an exception as well.
If you want that to work, you must insert a commit() call after line #1, and
LTW follows this logic.
bq. Also an explanation of what it's doing underneath
Refreshing LTR means reopening its internal IndexReader instance. If it has
changed, then LTR updates its parents array with the newly added categories.
Usually, assuming the taxonomy does not grow a lot (i.e., usually after some
point your taxonomy is relatively fixed, and new categories are not added often
-- much like an index lexicon), this additional update of the parents array is
quick.
> Add NRT support to LuceneTaxonomyReader
> ---
>
> Key: LUCENE-3441
> URL: https://issues.apache.org/jira/browse/LUCENE-3441
> Project: Lucene - Java
> Issue Type: New Feature
> Components: modules/facet
>Reporter: Shai Erera
>Priority: Minor
>
> Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to
> LuceneTaxonomyWriter, you cannot have the reader updated, like
> IndexReader/Writer. In order to do that we need to do the following:
> # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with
> LuceneTaxonomyWriter.
> # Add API to LuceneTaxonomyWriter to expose its internal IndexReader
> # Change LTR.refresh() to return an LTR, rather than void. This is actually
> not strictly related to that issue, but since we'll need to modify refresh()
> impl, I think it'll be good to change its API as well. Since all of facet API
> is @lucene.experimental, no backwards issues here (and the sooner we do it,
> the better).
--
This message is automatically generated by JIRA.
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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13109599#comment-13109599 ] Jason Rutherglen commented on LUCENE-3441: -- It would be great if the cost of (re)opening a new LTR is. Also an explanation of what it's doing underneath. > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Java > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Priority: Minor > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- This message is automatically generated by JIRA. 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
[jira] [Commented] (LUCENE-3441) Add NRT support to LuceneTaxonomyReader
[ https://issues.apache.org/jira/browse/LUCENE-3441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13109584#comment-13109584 ] Mihai Caraman commented on LUCENE-3441: --- Newb question: Shouldn't you also commit in the constructor, so you can create a reader right after? For exmaple, to work with the taxReader with refresh(), I have to initialize: taxWriter,commit,taxReader, else it throws no segment exception(which you'd expect to be there because of the taxWriter ctor, or is that just me:P ?). > Add NRT support to LuceneTaxonomyReader > --- > > Key: LUCENE-3441 > URL: https://issues.apache.org/jira/browse/LUCENE-3441 > Project: Lucene - Java > Issue Type: New Feature > Components: modules/facet >Reporter: Shai Erera >Priority: Minor > > Currently LuceneTaxonomyReader does not support NRT - i.e., on changes to > LuceneTaxonomyWriter, you cannot have the reader updated, like > IndexReader/Writer. In order to do that we need to do the following: > # Add ctor to LuceneTaxonomyReader to allow you to instantiate it with > LuceneTaxonomyWriter. > # Add API to LuceneTaxonomyWriter to expose its internal IndexReader > # Change LTR.refresh() to return an LTR, rather than void. This is actually > not strictly related to that issue, but since we'll need to modify refresh() > impl, I think it'll be good to change its API as well. Since all of facet API > is @lucene.experimental, no backwards issues here (and the sooner we do it, > the better). -- This message is automatically generated by JIRA. 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
