NightOwl888 commented on code in PR #847:
URL: https://github.com/apache/lucenenet/pull/847#discussion_r1180954177
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -181,11 +181,51 @@ public static void Unlock(Directory directory)
/// <exception cref="IOException">
/// if another error occurred. </exception>
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode,
- ITaxonomyWriterCache cache)
+ ITaxonomyWriterCache cache) : this(directory, openMode, cache, new
DirectoryTaxonomyIndexWriterFactory())
+ {
+ }
+
+ /// <summary>
+ /// Construct a Taxonomy writer.
+ /// </summary>
+ /// <param name="directory">
+ /// The <see cref="Store.Directory"/> in which to store the
taxonomy. Note that
+ /// the taxonomy is written directly to that directory (not to a
+ /// subdirectory of it). </param>
+ /// <param name="openMode">
+ /// Specifies how to open a taxonomy for writing: <see
cref="OpenMode.APPEND"/>
+ /// means open an existing index for append (failing if the index
does
+ /// not yet exist). <see cref="OpenMode.CREATE"/> means create a
new index (first
+ /// deleting the old one if it already existed).
+ /// <see cref="OpenMode.CREATE_OR_APPEND"/> appends to an existing
index if there
+ /// is one, otherwise it creates a new index. </param>
+ /// <param name="cache">
+ /// A <see cref="ITaxonomyWriterCache"/> implementation which
determines
+ /// the in-memory caching policy. See for example
+ /// <see cref="WriterCache.LruTaxonomyWriterCache"/> and <see
cref="Cl2oTaxonomyWriterCache"/>.
+ /// If null or missing, <see cref="DefaultTaxonomyWriterCache()"/>
is used. </param>
+ /// <param name="indexWriterFactory">
+ /// A <see cref="DirectoryTaxonomyIndexWriterFactory"/>
implementation that can be used to
+ /// customize the <see cref="IndexWriter"/> configuration and
writer itself that's used to
+ /// store the taxonomy index.</param>
+ /// <exception cref="CorruptIndexException">
+ /// if the taxonomy is corrupted. </exception>
+ /// <exception cref="LockObtainFailedException">
+ /// if the taxonomy is locked by another writer. If it is known
+ /// that no other concurrent writer is active, the lock might
+ /// have been left around by an old dead process, and should be
+ /// removed using <see cref="Unlock(Directory)"/>. </exception>
+ /// <exception cref="IOException">
+ /// if another error occurred. </exception>
+ /// <exception cref="System.ArgumentNullException"> if <paramref
name="indexWriterFactory"/> is <c>null</c> </exception>
+ protected DirectoryTaxonomyWriter(Directory directory, OpenMode
openMode,
+ ITaxonomyWriterCache cache, DirectoryTaxonomyIndexWriterFactory
indexWriterFactory)
{
dir = directory;
- IndexWriterConfig config = CreateIndexWriterConfig(openMode);
- indexWriter = OpenIndexWriter(dir, config);
+
+ if (indexWriterFactory == null) throw new
ArgumentNullException(nameof(indexWriterFactory));
Review Comment:
Please change to `indexWriterFactory is null` for this check.
##########
src/Lucene.Net.Replicator/IndexAndTaxonomyRevision.cs:
##########
@@ -44,35 +44,28 @@ namespace Lucene.Net.Replicator
public class IndexAndTaxonomyRevision : IRevision
{
/// <summary>
- /// A <see cref="DirectoryTaxonomyWriter"/> which sets the underlying
+ /// A <see cref="DirectoryTaxonomyIndexWriterFactory"/> which sets the
underlying
/// <see cref="Index.IndexWriter"/>'s <see
cref="IndexDeletionPolicy"/> to
/// <see cref="SnapshotDeletionPolicy"/>.
/// </summary>
- public class SnapshotDirectoryTaxonomyWriter : DirectoryTaxonomyWriter
+ public class SnapshotDirectoryTaxonomyIndexWriterFactory :
DirectoryTaxonomyIndexWriterFactory
Review Comment:
Being that we have already broken this API, let's de-nest the class as per
[CA1034](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1034).
And since we repurposed it, let's move it to the `Support` folder and mark it
`// LUCENENET specific`. But keep it in the same namespace as it is now.
Also, be sure to drop a comment here:
```c#
// LUCENENET specific - refactored SnapshotDirectoryTaxonomyWriter into
SnapshotDirectoryTaxonomyIndexWriterFactory and de-nested
```
##########
src/Lucene.Net.Tests.Facet/Taxonomy/Directory/TestDirectoryTaxonomyReader.cs:
##########
@@ -266,17 +266,9 @@ public virtual void TestOpenIfChangedManySegments()
dir.Dispose();
}
- private sealed class DirectoryTaxonomyWriterAnonymousClass :
DirectoryTaxonomyWriter
+ private sealed class TestDirectoryTaxonomyIndexWriterFactory :
DirectoryTaxonomyIndexWriterFactory
Review Comment:
Let's add a comment here
```c#
// LUCENENET specific: Converted DirectoryTaxonomyWriter anonymous class
into a DirectoryTaxonomyIndexWriterFactory subclass
```
##########
src/Lucene.Net.Tests.Facet/Taxonomy/Directory/TestDirectoryTaxonomyReader.cs:
##########
@@ -326,17 +313,29 @@ public virtual void TestOpenIfChangedMergedSegment()
dir.Dispose();
}
- private sealed class DirectoryTaxonomyWriterAnonymousClass2 :
DirectoryTaxonomyWriter
+ private sealed class TestDirectoryTaxonomyIndexWriterFactory2 :
DirectoryTaxonomyIndexWriterFactory
Review Comment:
Let's add a comment here
```c#
// LUCENENET specific: Converted DirectoryTaxonomyWriter anonymous class
into a DirectoryTaxonomyIndexWriterFactory subclass
```
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -181,11 +181,51 @@ public static void Unlock(Directory directory)
/// <exception cref="IOException">
/// if another error occurred. </exception>
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode,
- ITaxonomyWriterCache cache)
+ ITaxonomyWriterCache cache) : this(directory, openMode, cache, new
DirectoryTaxonomyIndexWriterFactory())
+ {
+ }
+
+ /// <summary>
+ /// Construct a Taxonomy writer.
+ /// </summary>
+ /// <param name="directory">
+ /// The <see cref="Store.Directory"/> in which to store the
taxonomy. Note that
+ /// the taxonomy is written directly to that directory (not to a
+ /// subdirectory of it). </param>
+ /// <param name="openMode">
+ /// Specifies how to open a taxonomy for writing: <see
cref="OpenMode.APPEND"/>
+ /// means open an existing index for append (failing if the index
does
+ /// not yet exist). <see cref="OpenMode.CREATE"/> means create a
new index (first
+ /// deleting the old one if it already existed).
+ /// <see cref="OpenMode.CREATE_OR_APPEND"/> appends to an existing
index if there
+ /// is one, otherwise it creates a new index. </param>
+ /// <param name="cache">
+ /// A <see cref="ITaxonomyWriterCache"/> implementation which
determines
+ /// the in-memory caching policy. See for example
+ /// <see cref="WriterCache.LruTaxonomyWriterCache"/> and <see
cref="Cl2oTaxonomyWriterCache"/>.
+ /// If null or missing, <see cref="DefaultTaxonomyWriterCache()"/>
is used. </param>
+ /// <param name="indexWriterFactory">
+ /// A <see cref="DirectoryTaxonomyIndexWriterFactory"/>
implementation that can be used to
+ /// customize the <see cref="IndexWriter"/> configuration and
writer itself that's used to
+ /// store the taxonomy index.</param>
+ /// <exception cref="CorruptIndexException">
+ /// if the taxonomy is corrupted. </exception>
+ /// <exception cref="LockObtainFailedException">
+ /// if the taxonomy is locked by another writer. If it is known
+ /// that no other concurrent writer is active, the lock might
+ /// have been left around by an old dead process, and should be
+ /// removed using <see cref="Unlock(Directory)"/>. </exception>
+ /// <exception cref="IOException">
+ /// if another error occurred. </exception>
+ /// <exception cref="System.ArgumentNullException"> if <paramref
name="indexWriterFactory"/> is <c>null</c> </exception>
+ protected DirectoryTaxonomyWriter(Directory directory, OpenMode
openMode,
Review Comment:
Let's move the 2 new constructor parameters over to the left side. This will
make the API seem more uniform, since the final 2 parameters are optional.
Then we will have these constructors:
```c#
public DirectoryTaxonomyWriter(Directory directory)
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode)
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode,
ITaxonomyWriterCache cache)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, Directory directory)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, Directory directory, OpenMode openMode)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, Directory directory, OpenMode openMode,
ITaxonomyWriterCache cache)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, IndexWriterFactory indexWriterFactory, Directory
directory)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, IndexWriterFactory indexWriterFactory, Directory
directory, OpenMode openMode)
public DirectoryTaxonomyWriter(IIndexWriterConfigFactory
indexWriterConfigFactory, IndexWriterFactory indexWriterFactory, Directory
directory, OpenMode openMode, ITaxonomyWriterCache cache)
```
I suspect that passing an `IndexWriterFactory` is going to be quite a rare
thing, but it can be useful if the user wants to subclass `IndexWriter`.
The latest version of Lucene makes all of the constructors public, so let's
follow suit.
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyIndexWriterFactory.cs:
##########
@@ -0,0 +1,79 @@
+using Lucene.Net.Index;
+using Lucene.Net.Index.Extensions;
+using Lucene.Net.Util;
+
+namespace Lucene.Net.Facet.Taxonomy.Directory
+{
+ /*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ using Directory = Lucene.Net.Store.Directory;
+
+ /// <summary>
+ /// This class offers some hooks for extending classes to control the
+ /// <see cref="IndexWriter"/> instance that is used by <see
cref="DirectoryTaxonomyWriter"/>.
+ /// </summary>
+ public class DirectoryTaxonomyIndexWriterFactory
Review Comment:
I made a wider analysis of this, and there is at least one more API we can
fix by generalizing a bit more.
https://github.com/apache/lucenenet/blob/1ae0ca15900d780ea951646c1ffd7729f3c625f4/src/Lucene.Net.Suggest/Suggest/Analyzing/AnalyzingInfixSuggester.cs#L182
Let's start with `IndexWriterConfig`. This can be generalized into the
following interface.
```c#
namespace Lucene.Net.Index
{
public interface IIndexWriterConfigFactory
{
IndexWriterConfig CreateIndexWriterConfig(LuceneVersion version,
Analyzer analyzer, OpenMode openMode);
}
}
```
Now, for the `IndexWriter`
```c#
namespace Lucene.Net.Index
{
public class IndexWriterFactory
{
public static IndexWriterFactory Default { get; } = new
IndexWriterFactory();
public virtual IndexWriter OpenIndexWriter(Directory directory,
IndexWriterConfig config)
{
return new IndexWriter(directory, config);
}
}
}
```
For `AnalyzingInfixSuggester`, we can then make a static property which will
be reused in the constructors.
```c#
public static IIndexWriterConfigFactory DefaultIndexWriterConfigFactory
{ get; }
= new AnalyzingInfixSuggesterIndexWriterConfigFactory(); // Or
perhaps a shorter name...
```
For `DirectoryTaxonomyWriter`, we can then make a static property which will
be reused in the constructors.
```c#
public static IIndexWriterConfigFactory DefaultIndexWriterConfigFactory
{ get; }
= new DirectoryTaxonomyIndexWriterConfigFactory(); // Or perhaps a
shorter name...
```
And create a private wrapper method to supply the extra 2 values to the
`IIndexWriterConfigFactory`:
```c#
private IndexWriterConfig CreateIndexWriterConfig(OpenMode openMode)
{
return
indexWriterConfigFactory.CreateIndexWriterConfig(LuceneVersion.LUCENE_48,
analyzer: null, openMode);
}
```
Then it is up to the `IIndexWriterConfigFactory` implementation whether to
use those passed in values or ignore them and supply values that are hard coded
or from the constructor instead.
Of course, these 2 extra parameters are more meaningful in the
`AnalyzingInfixSuggester` where they are actually passed into the constructor
of the class, but this seems like a reasonable compromise for code reuse.
Please use the above namespaces for these factories, but move them into the
`Lucene.Net` assembly in the `Lucene.Net/Support/Index` folder.
Also be sure to add `// LUCENENET specific` comments to them, although it
should be fairly obvious by moving them into that folder.
> NOTE: For `IndexReaderFactory`, let's go ahead and duplicate all 5
overloads of `DirectoryReader.Open()`. This will make it more general.
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -253,53 +293,6 @@ public static void Unlock(Directory directory)
}
}
- /// <summary>
Review Comment:
We didn't break any constructors, but this is definitely a breaking API
change, so let's mark the PR accordingly.
It is helpful for the release notes to summarize all of the breaking API
changes in this PR in the initial comments.
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -354,6 +347,12 @@ public static ITaxonomyWriterCache
DefaultTaxonomyWriterCache()
/// </summary>
public DirectoryTaxonomyWriter(Directory directory) : this(directory,
OpenMode.CREATE_OR_APPEND) { }
+ /// <summary>
+ /// Create this with <see cref="OpenMode.CREATE_OR_APPEND"/> and <see
cref="DefaultTaxonomyWriterCache()"/>.
+ /// </summary>
+ public DirectoryTaxonomyWriter(Directory directory,
DirectoryTaxonomyIndexWriterFactory indexWriterFactory)
Review Comment:
Just above this there is a `DefaultTaxonomyWriterCache()` method, which is a
bit confusing because it doesn't have a verb. Let's rename it
`CreateDefaultTaxonomyWriterCache()` since it returns a new instance on each
call and is therefore not a good candidate for a property.
##########
.gitignore:
##########
@@ -62,4 +62,7 @@ websites/apidocs/api/**/*.manifest
!websites/apidocs/api/toc.yml
# Apache Releases on Subversion
-svn-*/
\ No newline at end of file
+svn-*/
+
+# vscode files
+.vscode/
Review Comment:
Admittedly, I don't have much experience with VSCode. However, it seemed
like a good idea to add a `.vscode/` directory to the project so it would be
easier for users of VSCode to load and run the tests. However, I am not sure
that is what we have in the `.vscode/` directory.
What is the reason we would exclude this? Does the project "just work" in a
way VSCode users might expect?
Given our goal is to make it as easy as possible for users of VSCode to
contribute, do you have any suggestions on how we can improve the experience?
> Side note: Perhaps we should revert this here and make VSCode usability
into a separate PR.
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -181,11 +181,51 @@ public static void Unlock(Directory directory)
/// <exception cref="IOException">
/// if another error occurred. </exception>
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode,
- ITaxonomyWriterCache cache)
+ ITaxonomyWriterCache cache) : this(directory, openMode, cache, new
DirectoryTaxonomyIndexWriterFactory())
+ {
+ }
+
+ /// <summary>
+ /// Construct a Taxonomy writer.
+ /// </summary>
+ /// <param name="directory">
+ /// The <see cref="Store.Directory"/> in which to store the
taxonomy. Note that
+ /// the taxonomy is written directly to that directory (not to a
+ /// subdirectory of it). </param>
+ /// <param name="openMode">
+ /// Specifies how to open a taxonomy for writing: <see
cref="OpenMode.APPEND"/>
+ /// means open an existing index for append (failing if the index
does
+ /// not yet exist). <see cref="OpenMode.CREATE"/> means create a
new index (first
+ /// deleting the old one if it already existed).
+ /// <see cref="OpenMode.CREATE_OR_APPEND"/> appends to an existing
index if there
+ /// is one, otherwise it creates a new index. </param>
+ /// <param name="cache">
+ /// A <see cref="ITaxonomyWriterCache"/> implementation which
determines
+ /// the in-memory caching policy. See for example
+ /// <see cref="WriterCache.LruTaxonomyWriterCache"/> and <see
cref="Cl2oTaxonomyWriterCache"/>.
+ /// If null or missing, <see cref="DefaultTaxonomyWriterCache()"/>
is used. </param>
+ /// <param name="indexWriterFactory">
+ /// A <see cref="DirectoryTaxonomyIndexWriterFactory"/>
implementation that can be used to
+ /// customize the <see cref="IndexWriter"/> configuration and
writer itself that's used to
+ /// store the taxonomy index.</param>
+ /// <exception cref="CorruptIndexException">
+ /// if the taxonomy is corrupted. </exception>
+ /// <exception cref="LockObtainFailedException">
+ /// if the taxonomy is locked by another writer. If it is known
+ /// that no other concurrent writer is active, the lock might
+ /// have been left around by an old dead process, and should be
+ /// removed using <see cref="Unlock(Directory)"/>. </exception>
+ /// <exception cref="IOException">
+ /// if another error occurred. </exception>
+ /// <exception cref="System.ArgumentNullException"> if <paramref
name="indexWriterFactory"/> is <c>null</c> </exception>
+ protected DirectoryTaxonomyWriter(Directory directory, OpenMode
openMode,
Review Comment:
Please remove the space after `openMode,`
##########
src/Lucene.Net.Tests.Facet/Taxonomy/Directory/TestDirectoryTaxonomyReader.cs:
##########
@@ -352,15 +351,10 @@ public virtual void
TestOpenIfChangedNoChangesButSegmentMerges()
// hold onto IW to forceMerge
// note how we don't close it, since DTW will close it.
- var iw = new IndexWriter(dir, new
IndexWriterConfig(TEST_VERSION_CURRENT, new
MockAnalyzer(Random)).SetMergePolicy(new LogByteSizeMergePolicy()));
Review Comment:
Let's drop a comment here so it is clear what we did.
```c#
// LUCENENET: Moved the creation of IndexWriter to
TestDirectoryTaxonomyIndexWriterFactory2
```
##########
src/Lucene.Net.Tests.Facet/Taxonomy/Directory/TestDirectoryTaxonomyReader.cs:
##########
@@ -231,7 +231,7 @@ public virtual void TestOpenIfChangedManySegments()
// test openIfChanged() when the taxonomy contains many segments
Directory dir = NewDirectory();
- DirectoryTaxonomyWriter writer = new
DirectoryTaxonomyWriterAnonymousClass(this, dir);
+ var writer = new DirectoryTaxonomyWriter(dir, new
TestDirectoryTaxonomyIndexWriterFactory());
Review Comment:
Let's add a comment here:
```c#
// LUCENENET specific: Added DirectoryTaxonomyIndexWriterFactory constructor
parameter
```
##########
src/Lucene.Net.Facet/Taxonomy/Directory/DirectoryTaxonomyWriter.cs:
##########
@@ -181,11 +181,51 @@ public static void Unlock(Directory directory)
/// <exception cref="IOException">
/// if another error occurred. </exception>
public DirectoryTaxonomyWriter(Directory directory, OpenMode openMode,
Review Comment:
Please remove the space after `openMode,`
##########
src/Lucene.Net.Tests.Facet/Taxonomy/Directory/TestDirectoryTaxonomyReader.cs:
##########
@@ -295,14 +287,9 @@ public virtual void TestOpenIfChangedMergedSegment()
// hold onto IW to forceMerge
// note how we don't close it, since DTW will close it.
- IndexWriter iw = new IndexWriter(dir, new
IndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(Random))
Review Comment:
Let's drop a comment here so it is clear what we did.
```c#
// LUCENENET: Moved the creation of IndexWriter to
TestDirectoryTaxonomyIndexWriterFactory2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
