NightOwl888 commented on code in PR #1014:
URL: https://github.com/apache/lucenenet/pull/1014#discussion_r1835668053
##########
src/Lucene.Net.Tests/Index/TestAddIndexes.cs:
##########
@@ -665,7 +741,7 @@ public RunAddIndexesThreads(TestAddIndexes outerInstance,
int numCopy)
}
}
- internal virtual void LaunchThreads(int numIter)
+ internal void LaunchThreads(int numIter)
Review Comment:
In Java, unspecified methods are virtual by default and must be explicitly
set to `final` to make non-virtual. So, this accessibility no longer matches.
Please revert this change.
##########
src/Lucene.Net.Tests/Index/TestBackwardsCompatibility.cs:
##########
@@ -310,7 +308,7 @@ public virtual void TestUnsupportedOldIndexes()
indexStatus = checker.DoCheckIndex();
}
Assert.IsFalse(indexStatus.Clean);
-
Assert.IsTrue(sb.ToString().Contains(typeof(IndexFormatTooOldException).Name));
+
Assert.IsTrue(sb.ToString().Contains(nameof(IndexFormatTooOldException)));
Review Comment:
Minor performance improvement: Please allocate 512 chars (1024 bytes) for
`sb` as was the case upstream. Also, rename the variable `sb` to `bos` to match.
##########
src/Lucene.Net/Index/IndexWriter.cs:
##########
@@ -195,23 +195,23 @@ public class IndexWriter : IDisposable, ITwoPhaseCommit
/// <summary>
/// Name of the write lock in the index.
/// </summary>
- public static readonly string WRITE_LOCK_NAME = "write.lock";
+ public const string WRITE_LOCK_NAME = "write.lock";
Review Comment:
Let's either move these changes to a new PR or open a new issue about them
(whichever is easier). Given that these are breaking changes, let's consider
them separately from the test review.
##########
src/Lucene.Net.Tests/Index/TestBackwardsCompatibility3x.cs:
##########
@@ -113,18 +112,18 @@ public void testCreateSingleSegmentNoCFS() throws
IOException {
// LUCENENET specific to load resources for this type
internal const string CURRENT_RESOURCE_DIRECTORY =
"Lucene.Net.Tests.Index.";
- internal static readonly string[] oldNames = new string[] {
+ internal static readonly string[] oldNames = {
Review Comment:
Please change the formatting of these 3 arrays to match Lucene. The list
approach seems simpler to visually parse.
https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/test/org/apache/lucene/index/TestBackwardsCompatibility3x.java#L112-L140
##########
src/Lucene.Net.Tests/Index/TestAddIndexes.cs:
##########
@@ -718,21 +794,21 @@ public override void Run()
}
}
- internal virtual void JoinThreads()
+ internal void JoinThreads()
Review Comment:
In Java, unspecified methods are virtual by default and must be explicitly
set to `final` to make non-virtual. So, this accessibility no longer matches.
Please revert this change.
##########
src/Lucene.Net.Tests/Index/TestBackwardsCompatibility.cs:
##########
@@ -158,18 +158,18 @@ public void testCreateMoreTermsIndex() throws Exception {
}
*/
- internal static readonly string[] oldNames = new string[] {
+ internal static readonly string[] oldNames = {
Review Comment:
Please change the formatting of these 3 arrays to match Lucene. The list
approach seems simpler to visually parse.
https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java#L164-L194
##########
src/Lucene.Net.Tests/Index/TestAddIndexes.cs:
##########
@@ -718,21 +794,21 @@ public override void Run()
}
}
- internal virtual void JoinThreads()
+ internal void JoinThreads()
{
for (int i = 0; i < NUM_THREADS; i++)
{
threads[i].Join();
}
}
- internal virtual void Close(bool doWait)
+ internal void Close(bool doWait)
{
didClose = true;
writer2.Dispose(doWait);
}
- internal virtual void CloseDir()
+ internal void CloseDir()
Review Comment:
In Java, unspecified methods are virtual by default and must be explicitly
set to `final` to make non-virtual. So, this accessibility no longer matches.
Please revert this change.
##########
src/Lucene.Net.Tests/Index/TestAddIndexes.cs:
##########
@@ -718,21 +794,21 @@ public override void Run()
}
}
- internal virtual void JoinThreads()
+ internal void JoinThreads()
{
for (int i = 0; i < NUM_THREADS; i++)
{
threads[i].Join();
}
}
- internal virtual void Close(bool doWait)
+ internal void Close(bool doWait)
Review Comment:
In Java, unspecified methods are virtual by default and must be explicitly
set to `final` to make non-virtual. So, this accessibility no longer matches.
Please revert this change.
##########
src/Lucene.Net.Tests/Index/TestDirectoryReader.cs:
##########
@@ -349,19 +350,20 @@ public virtual void TestTermVectors()
d.Dispose();
}
- internal virtual void AssertTermDocsCount(string msg, IndexReader
reader, Term term, int expected)
- {
- DocsEnum tdocs = TestUtil.Docs(Random, reader, term.Field, new
BytesRef(term.Text), MultiFields.GetLiveDocs(reader), null, 0);
- int count = 0;
- if (tdocs != null)
- {
- while (tdocs.NextDoc() != DocIdSetIterator.NO_MORE_DOCS)
- {
- count++;
- }
- }
- Assert.AreEqual(expected, count, msg + ", count mismatch");
- }
+ // LUCENENET specific - commented out unused method
Review Comment:
Good catch, however I find myself tending to update commented out code quite
a bit. Let's just delete the method and leave a comment indicating that we
deleted it because it was not in use.
```c#
// LUCENENET specific - Removed AssertTermDocsCount() because it was not in
use
```
##########
src/Lucene.Net.Tests/Index/TestBackwardsCompatibility3x.cs:
##########
@@ -240,7 +238,7 @@ public virtual void TestUnsupportedOldIndexes()
CheckIndex.Status indexStatus = checker.DoCheckIndex();
Assert.IsFalse(indexStatus.Clean);
checker.InfoStream.Flush();
-
Assert.IsTrue(bos.ToString().Contains(typeof(IndexFormatTooOldException).Name));
+
Assert.IsTrue(bos.ToString().Contains(nameof(IndexFormatTooOldException)));
Review Comment:
Minor performance improvement: Please allocate 512 chars (1024 bytes) for
`bos` as was the case upstream.
##########
src/Lucene.Net.Tests/Index/TestDeletionPolicy.cs:
##########
@@ -40,11 +38,10 @@ namespace Lucene.Net.Index
using TermQuery = Lucene.Net.Search.TermQuery;
using TestUtil = Lucene.Net.Util.TestUtil;
- /*
- Verify we can read the pre-2.1 file format, do searches
- against it, and add documents to it.
- */
-
+ /// <summary>
+ /// Verify we can read the pre-2.1 file format, do searches
+ /// against it, and add documents to it.
+ /// </summary>
[TestFixture]
public class TestDeletionPolicy : LuceneTestCase
{
Review Comment:
Looks like we could make `VerifyCommitOrder()` static here and then we can
eliminate the `outerInstance` in each of the nested classes that call it.
##########
src/Lucene.Net.Tests/Index/TestBinaryDocValuesUpdates.cs:
##########
@@ -1142,46 +1137,17 @@ public virtual void TestUpdateOldSegments()
doc.Add(new BinaryDocValuesField("f", ToBytes(5L)));
writer.AddDocument(doc);
writer.Dispose();
- dir.Dispose();
- }
- [Test, LuceneNetSpecific]
- public virtual void TestUpdateOldSegments_OldFormatNotActive()
- {
- bool oldValue = OldFormatImpersonationIsActive;
-
- OldFormatImpersonationIsActive = false;
-
- Codec[] oldCodecs = new Codec[] {
- new Lucene40RWCodec(),
- new Lucene41RWCodec(),
- new Lucene42RWCodec(),
- new Lucene45RWCodec()
- };
-
- Directory dir = NewDirectory();
- Document doc = new Document();
- doc.Add(new StringField("id", "doc", Store.NO));
- doc.Add(new BinaryDocValuesField("f", ToBytes(5L)));
-
- var conf = NewIndexWriterConfig(TEST_VERSION_CURRENT, new
MockAnalyzer(Random));
- conf.SetCodec(oldCodecs[Random.Next(oldCodecs.Length)]);
-
- var writer = new IndexWriter(dir, conf);
- writer.AddDocument(doc);
+ conf = NewIndexWriterConfig(TEST_VERSION_CURRENT, new
MockAnalyzer(Random));
+ writer = new IndexWriter(dir, conf);
writer.UpdateBinaryDocValue(new Term("id", "doc"), "f",
ToBytes(4L));
-
- try
- {
+ OldFormatImpersonationIsActive = false;
+ try {
Review Comment:
Good catch!
This is the only spot I am aware of where we are using this style of curly
brackets, but it is a minor issue. So, I will leave it up to you whether to fix
it or leave it as-is.
##########
src/Lucene.Net.Tests/Index/TestDirectoryReaderReopen.cs:
##########
@@ -393,7 +386,7 @@ public override void Run()
if (index % 2 == 0)
{
// refresh reader synchronized
- ReaderCouple c = (outerInstance.RefreshReader(r, test,
index, true));
+ ReaderCouple c = outerInstance.RefreshReader(r, test,
index, true);
Review Comment:
Looks like we can make both overloads of `RefreshReader()` static so we can
eliminate this `outerInstance`. It only has one external dependency,
`createReaderMutex`, which can also be made static.
##########
src/Lucene.Net/Util/ByteBlockPool.cs:
##########
@@ -45,9 +45,9 @@ namespace Lucene.Net.Util
/// </summary>
public sealed class ByteBlockPool
{
- public static readonly int BYTE_BLOCK_SHIFT = 15;
- public static readonly int BYTE_BLOCK_SIZE = 1 << BYTE_BLOCK_SHIFT;
- public static readonly int BYTE_BLOCK_MASK = BYTE_BLOCK_SIZE - 1;
+ public const int BYTE_BLOCK_SHIFT = 15;
Review Comment:
Let's either move these changes to a new PR or open a new issue about them
(whichever is easier). Given that these are breaking changes, let's consider
them separately from the test review.
##########
src/Lucene.Net.Tests/Index/TestIndexWriterReader.cs:
##########
@@ -757,8 +759,7 @@ public virtual void TestMergeWarmer()
((LogMergePolicy)writer.Config.MergePolicy).MergeFactor = 2;
- //int num = AtLeast(100);
- int num = 101;
+ int num = AtLeast(100);
Review Comment:
Good catch!
--
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: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
