lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From NightOwl888 <...@git.apache.org>
Subject [GitHub] lucenenet issue #188: Fixed 64 Failing Facet Tests and Finished Facet Implem...
Date Sun, 02 Oct 2016 06:21:56 GMT
Github user NightOwl888 commented on the issue:

    https://github.com/apache/lucenenet/pull/188
  
    @eladmarg 
    
    That's great news. 
    
    I took a look and it seems okay. [Here is my branch](https://github.com/NightOwl888/lucenenet/tree/facet-lurch-table)
showing it functioning.
    
    1. [NameIntCacheLRU was updated](https://github.com/NightOwl888/lucenenet/blob/facet-lurch-table/src/Lucene.Net.Facet/Taxonomy/WriterCache/NameIntCacheLRU.cs#L73)
to use `LurchTable` for LRU, and it is now passing all of the concurrency tests.
    2. [LRUHashMap](https://github.com/NightOwl888/lucenenet/blob/facet-lurch-table/src/Lucene.Net.Facet/Taxonomy/LRUHashMap.cs)
is now just an adapter around `LurchTable` so we get the right `Get` and `Put` behavior to
match Java.
    3. [I added some unit tests](https://github.com/NightOwl888/lucenenet/blob/facet-lurch-table/src/Lucene.Net.Tests.Facet/Taxonomy/TestLRUHashMap.cs#L63-L67)
to demonstrate that the `Put` behavior functions properly. Although it took some work to figure
out out how to achieve it, it turned out to be [quite simple](https://github.com/NightOwl888/lucenenet/blob/facet-lurch-table/src/Lucene.Net.Facet/Taxonomy/LRUHashMap.cs#L80-L86)
to mimic Java's behavior in a thread-safe way.
    
    However, there are a couple of issues:
    
    1. The `Limit` property is readonly, but [in Lucene it needs to be changed](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/4.8.0/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/directory/DirectoryTaxonomyReader.java#L330-L347)
(increased) dynamically. So, for now I have it [throwing a `NotImplementedException`](https://github.com/NightOwl888/lucenenet/blob/facet-lurch-table/src/Lucene.Net.Facet/Taxonomy/LRUHashMap.cs#L69-L74).
    2. We are getting this Lucene.Net implementation to work on .NET core, and there is currently
no support for it with `CSharpTest.Net.Collections`.
    
    The first issue can be easily patched. We could ultimately go back to putting the locks
into DirectoryTaxonomyReader to update that property safely.
    
    However, the second issue isn't something we can ignore. If there aren't too many dependencies,
it might make the most sense to bring the code over into our Support namespace. 
    
    If not, our only option may be to fork it, patch it, rename it, and post it on NuGet since
the code hasn't been touched in over 2 years, there was only 1 release, and it is not very
likely we will be able to get the original owner to update it (although we should try that
first). I don't imagine getting a set of data collections to work on .NET core will be too
much of a challenge.
    
    Still, despite these issues, this is looking like our best option, because it is thread-safe.
    
    The alternative would be to port `LinkedHashMap` from Java. While it is only ~750 lines
of code, half of which are comments, its base class is `HashMap` and I am not sure how well
it will fit with our current .NETified `HashMap` implementation (which doesn't have `Put`
or `Get` methods). But even the author of the Lucene LRUHashMap [said it would be better if
it were thread-safe rather than going this route](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/4.8.0/lucene/facet/src/java/org/apache/lucene/facet/taxonomy/LRUHashMap.java#L42-L48).
    
    @synhershko - what are your thoughts on this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message