Lucene - Core
  1. Lucene - Core
  2. LUCENE-4532

TestDirectoryTaxonomyReader.testRefreshReadRecreatedTaxonomy failure

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1, 6.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      The following failure on Jenkins:

      > Build: http://jenkins.sd-datasolutions.de/job/Lucene-Solr-4.x-Windows/1404/
      > Java: 32bit/jdk1.6.0_37 -client -XX:+UseConcMarkSweepGC
      >
      > 1 tests failed.
      > REGRESSION:  org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.testRefreshReadRecreatedTaxonomy
      >
      > Error Message:
      >
      >
      > Stack Trace:
      > java.lang.ArrayIndexOutOfBoundsException
      >         at __randomizedtesting.SeedInfo.seed([6AB10D3E4E956CFA:BFB2863DB7E077E0]:0)
      >         at java.lang.System.arraycopy(Native Method)
      >         at org.apache.lucene.facet.taxonomy.directory.ParentArray.refresh(ParentArray.java:99)
      >         at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyReader.refresh(DirectoryTaxonomyReader.java:407)
      >         at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.doTestReadRecreatedTaxono(TestDirectoryTaxonomyReader.java:167)
      >         at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.testRefreshReadRecreatedTaxonomy(TestDirectoryTaxonomyReader.java:130)
      >         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      >         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      >         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      >         at java.lang.reflect.Method.invoke(Method.java:597)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1559)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner.access$600(RandomizedRunner.java:79)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:737)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:773)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:787)
      >         at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:50)
      >         at org.apache.lucene.util.TestRuleFieldCacheSanity$1.evaluate(TestRuleFieldCacheSanity.java:51)
      >         at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
      >         at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
      >         at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)
      >         at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
      >         at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
      >         at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      >         at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
      >         at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:782)
      >         at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:442)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:746)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$3.evaluate(RandomizedRunner.java:648)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$4.evaluate(RandomizedRunner.java:682)
      >         at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:693)
      >         at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
      >         at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42)
      >         at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
      >         at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
      >         at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
      >         at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      >         at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43)
      >         at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
      >         at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
      >         at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55)
      >         at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      >         at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
      >         at java.lang.Thread.run(Thread.java:662)
      >
      >
      >
      >
      > Build Log:
      > [...truncated 5664 lines...]
      > [junit4:junit4] Suite: org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader
      > [junit4:junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestDirectoryTaxonomyReader -Dtests.method=testRefreshReadRecreatedTaxonomy -Dtests.seed=6AB10D3E4E956CFA -Dtests.slow=true -Dtests.locale=fr_CA -Dtests.timezone=Atlantic/Jan_Mayen -Dtests.file.encoding=US-ASCII
      > [junit4:junit4] ERROR   0.06s | TestDirectoryTaxonomyReader.testRefreshReadRecreatedTaxonomy <<<
      > [junit4:junit4]    > Throwable #1: java.lang.ArrayIndexOutOfBoundsException
      > [junit4:junit4]    >    at __randomizedtesting.SeedInfo.seed([6AB10D3E4E956CFA:BFB2863DB7E077E0]:0)
      > [junit4:junit4]    >    at java.lang.System.arraycopy(Native Method)
      > [junit4:junit4]    >    at org.apache.lucene.facet.taxonomy.directory.ParentArray.refresh(ParentArray.java:99)
      > [junit4:junit4]    >    at org.apache.lucene.facet.taxonomy.directory.DirectoryTaxonomyReader.refresh(DirectoryTaxonomyReader.java:407)
      > [junit4:junit4]    >    at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.doTestReadRecreatedTaxono(TestDirectoryTaxonomyReader.java:167)
      > [junit4:junit4]    >    at org.apache.lucene.facet.taxonomy.directory.TestDirectoryTaxonomyReader.testRefreshReadRecreatedTaxonomy(TestDirectoryTaxonomyReader.java:130)
      > [junit4:junit4]    >    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      > [junit4:junit4]    >    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      > [junit4:junit4]    >    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      > [junit4:junit4]    >    at java.lang.reflect.Method.invoke(Method.java:597)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1559)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner.access$600(RandomizedRunner.java:79)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:737)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:773)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:787)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:50)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleFieldCacheSanity$1.evaluate(TestRuleFieldCacheSanity.java:51)
      > [junit4:junit4]    >    at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:782)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:442)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:746)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$3.evaluate(RandomizedRunner.java:648)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$4.evaluate(RandomizedRunner.java:682)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:693)
      > [junit4:junit4]    >    at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
      > [junit4:junit4]    >    at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
      > [junit4:junit4]    >    at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
      > [junit4:junit4]    >    at java.lang.Thread.run(Thread.java:662)
      > [junit4:junit4]   2> NOTE: test params are: codec=SimpleText, sim=RandomSimilarityProvider(queryNorm=true,coord=crazy): {}, locale=fr_CA, timezone=Atlantic/Jan_Mayen
      > [junit4:junit4]   2> NOTE: Windows 7 6.1 x86/Sun Microsystems Inc. 1.6.0_37 (32-bit)/cpus=2,threads=1,free=59103720,total=93417472
      > [junit4:junit4]   2> NOTE: All tests run in this JVM: [ObjectToIntMapTest, UnsafeByteArrayInputStreamTest, IntArrayTest, EncodingTest, CustomAssociationPropertyTest, TestDirectoryTaxonomyWriter, FacetSearchParamsTest, DefaultEnhancementsIndexingParamsTest, TestCategoryListCache, TestTopKInEachNodeResultHandler, AdaptiveAccumulatorTest, Vint8Test, TestScoredDocIdCollector, IntToIntMapTest, AssociationPropertyTest, TestMultiCLExample, ArrayHashMapTest, CategoryListIteratorTest, TestTopKResultsHandlerRandom, TestTotalFacetCounts, SamplingWrapperTest, TestCharBlockArray, IntToDoubleMapTest, TestFacetsCollector, TestFacetsAccumulatorWithComplement, SamplingAccumulatorTest, TestTopKResultsHandler, CategoryAttributesIterableTest, CategoryAttributesStreamTest, TestFacetArrays, TestCompactLabelToOrdinal, CategoryListPayloadStreamTest, CategoryTokenizerTest, TestAddTaxonomy, TestTotalFacetCountsCache, TestTaxonomyCombined, FacetsPayloadProcessorProviderTest, OrdinalPolicyTest, PathPolicyTest, TestScoredDocIDsUtils, TestDirectoryTaxonomyReader]
      > [junit4:junit4] Completed in 0.11s, 7 tests, 1 error <<< FAILURES!
      >
      > [...truncated 81 lines...]
      > BUILD FAILED
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\build.xml:335: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\build.xml:39: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\lucene\build.xml:519: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\lucene\common-build.xml:1691: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\lucene\module-build.xml:61: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\lucene\common-build.xml:1163: The following error occurred while executing this line:
      > C:\Users\JenkinsSlave\workspace\Lucene-Solr-4.x-Windows\lucene\common-build.xml:827: There were test failures: 65 suites, 264 tests, 1 error, 2 ignored (1 assumption)
      >
      > Total time: 18 minutes 43 seconds
      > Build step 'Invoke Ant' marked build as failure
      > Archiving artifacts
      > Recording test results
      > Description set: Java: 32bit/jdk1.6.0_37 -client -XX:+UseConcMarkSweepGC
      > Email was triggered for: Failure
      > Sending email for trigger: Failure
      
      1. LUCENE-4532.patch
        19 kB
        Shai Erera
      2. LUCENE-4532.patch
        17 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Doesn't reproduce on my machine. I tried with -Dtests.iters=100 and still not reproducing. Will dig in the code ...

        Show
        Shai Erera added a comment - Doesn't reproduce on my machine. I tried with -Dtests.iters=100 and still not reproducing. Will dig in the code ...
        Hide
        Shai Erera added a comment -

        The test isn't even concurrent , so why can't I reproduce !? Anyway, continue digging ...

        Show
        Shai Erera added a comment - The test isn't even concurrent , so why can't I reproduce !? Anyway, continue digging ...
        Hide
        Dawid Weiss added a comment -

        This looks like some sort of race condition – I can't reproduce either but I can't get the code to even enter the block which caused this failure; in ParentArray:

            int num = indexReader.maxDoc();
            if (prefetchParentOrdinal==null) {
                ...
            } else {
                HERE?!
            }
        

        Perhaps if you try to make it enter that block it'll be more repeatable.

        Show
        Dawid Weiss added a comment - This looks like some sort of race condition – I can't reproduce either but I can't get the code to even enter the block which caused this failure; in ParentArray: int num = indexReader.maxDoc(); if (prefetchParentOrdinal== null ) { ... } else { HERE?! } Perhaps if you try to make it enter that block it'll be more repeatable.
        Hide
        Shai Erera added a comment -

        Hmmm ... perhaps this explains the bug and why I cannot reproduce it. Here's a code snippet from DirTaxoReader.refresh():

            String t1 = indexReader.getIndexCommit().getUserData().get(DirectoryTaxonomyWriter.INDEX_CREATE_TIME);
            String t2 = r2.getIndexCommit().getUserData().get(DirectoryTaxonomyWriter.INDEX_CREATE_TIME);
            if (t1==null) {
              if (t2!=null) {
                r2.close();
                throw new InconsistentTaxonomyException("Taxonomy was recreated at: "+t2);
              }
            } else if (!t1.equals(t2)) {
              r2.close();
              throw new InconsistentTaxonomyException("Taxonomy was recreated at: "+t2+"  !=  "+t1);
            }
        

        The code compares the commit data that is known to the indexReader instance at hand, and the reopened one. If they do not match, it throws an exception, as this TaxoReader cannot be refreshed.

        The commit data relies on timestamp, which is a bad thing in general, even if the timestamp is in nano-second granularity. So what happens if a machine is able to add a category and commit DirTW in the same nano-second? Or maybe just open DirTW with OpenMode.CREATE and close it? Or the clocks somehow get messed up? The two timestamps in the commit data may accidentally be the same, leading to the exception exposed by the stacktrace above.

        I tried to modify the test to always use RAMDirectory() and not add any categories, just recreate+close, but I couldn't reproduce it either. However, I tried that on my laptop, and the timestamp is nano-second, so it may not be fast enough.

        Still, I think that that is a potential bug and may explain the errors that we see. The test is single threaded, the seed ensures we reproduce everything right, so all is left to blame is the machine's clock .

        I'll try to verify that a bit more, and on other machines, but I think that we should move to store an increment version number instead of timestamp.

        Show
        Shai Erera added a comment - Hmmm ... perhaps this explains the bug and why I cannot reproduce it. Here's a code snippet from DirTaxoReader.refresh(): String t1 = indexReader.getIndexCommit().getUserData().get(DirectoryTaxonomyWriter.INDEX_CREATE_TIME); String t2 = r2.getIndexCommit().getUserData().get(DirectoryTaxonomyWriter.INDEX_CREATE_TIME); if (t1== null ) { if (t2!= null ) { r2.close(); throw new InconsistentTaxonomyException( "Taxonomy was recreated at: " +t2); } } else if (!t1.equals(t2)) { r2.close(); throw new InconsistentTaxonomyException( "Taxonomy was recreated at: " +t2+ " != " +t1); } The code compares the commit data that is known to the indexReader instance at hand, and the reopened one. If they do not match, it throws an exception, as this TaxoReader cannot be refreshed. The commit data relies on timestamp, which is a bad thing in general, even if the timestamp is in nano-second granularity. So what happens if a machine is able to add a category and commit DirTW in the same nano-second? Or maybe just open DirTW with OpenMode.CREATE and close it? Or the clocks somehow get messed up? The two timestamps in the commit data may accidentally be the same, leading to the exception exposed by the stacktrace above. I tried to modify the test to always use RAMDirectory() and not add any categories, just recreate+close, but I couldn't reproduce it either. However, I tried that on my laptop, and the timestamp is nano-second, so it may not be fast enough. Still, I think that that is a potential bug and may explain the errors that we see. The test is single threaded, the seed ensures we reproduce everything right, so all is left to blame is the machine's clock . I'll try to verify that a bit more, and on other machines, but I think that we should move to store an increment version number instead of timestamp.
        Hide
        Shai Erera added a comment -

        This looks like some sort of race condition

        As I wrote, the test is single-threaded and very simple, so there's no race condition here. Also, the method (DTR.refresh()) is synchronized.

        I can't get the code to even enter the block which caused this failure

        That's why you and I cannot reproduce it. If the problem is indeed as I described (related to the time measurement), then we won't be able to reproduce it (most likely) b/c the code will always (that's what the test counts on) fail fast by throwing the exception.

        Show
        Shai Erera added a comment - This looks like some sort of race condition As I wrote, the test is single-threaded and very simple, so there's no race condition here. Also, the method (DTR.refresh()) is synchronized. I can't get the code to even enter the block which caused this failure That's why you and I cannot reproduce it. If the problem is indeed as I described (related to the time measurement), then we won't be able to reproduce it (most likely) b/c the code will always (that's what the test counts on) fail fast by throwing the exception.
        Hide
        Shai Erera added a comment -

        Patch contains the following fixes:

        • Addresses the current presumed test failure by moving to track taxonomy 'epoch' rather than creation time. The epoch is very similar to an index generation, only it is incremented whenever the taxonomy is re-created or replaced with another.
        • Addresses a possible race-condition in the constructor – now the code first opens the IndexWriter to obtain the lock on the index directory, and only then reads the commitData.

        Facet tests pass. Patch is against 4x.

        Show
        Shai Erera added a comment - Patch contains the following fixes: Addresses the current presumed test failure by moving to track taxonomy 'epoch' rather than creation time. The epoch is very similar to an index generation, only it is incremented whenever the taxonomy is re-created or replaced with another. Addresses a possible race-condition in the constructor – now the code first opens the IndexWriter to obtain the lock on the index directory, and only then reads the commitData. Facet tests pass. Patch is against 4x.
        Hide
        Michael McCandless added a comment -

        +1

        Very nice to remove relying on timestamps ...

        Show
        Michael McCandless added a comment - +1 Very nice to remove relying on timestamps ...
        Hide
        Shai Erera added a comment -

        Patch applies against trunk as-is. Only CHANGES entry need to be moved to the 4.1.0 section, which I'll do before I commit. If there are no objections, I'd like to commit it tomorrow.

        Show
        Shai Erera added a comment - Patch applies against trunk as-is. Only CHANGES entry need to be moved to the 4.1.0 section, which I'll do before I commit. If there are no objections, I'd like to commit it tomorrow.
        Hide
        Gilad Barkai added a comment -

        Reviewed the patch - and it looks very good.

        A few comments:
        1. in TestDirectoryTaxonomyWriter.java, the error string "index.create.time not found in commitData" should be updated.
        2. if the index creation time is in the commit data, it will not be removed - as the epoch is added to whatever commit data was read from the index. I think perhaps it should be removed?
        3. since the members related to the old 'timestamp' method are removed - no test could check the migration from old to new methods. Might be a good idea to add one with a comment to remove it when backward compatibility is no longer required (Lucene 6?).
        4. 'Epoch' is usually in the context of time, or in relation of a period. Perhaps the name 'version' is more closely related to the implementation?

        Show
        Gilad Barkai added a comment - Reviewed the patch - and it looks very good. A few comments: 1. in TestDirectoryTaxonomyWriter.java, the error string "index.create.time not found in commitData" should be updated. 2. if the index creation time is in the commit data, it will not be removed - as the epoch is added to whatever commit data was read from the index. I think perhaps it should be removed? 3. since the members related to the old 'timestamp' method are removed - no test could check the migration from old to new methods. Might be a good idea to add one with a comment to remove it when backward compatibility is no longer required (Lucene 6?). 4. 'Epoch' is usually in the context of time, or in relation of a period. Perhaps the name 'version' is more closely related to the implementation?
        Hide
        Shai Erera added a comment -

        Thanks for the review.

        in TestDirectoryTaxonomyWriter.java, the error string "index.create.time not found in commitData" should be updated.

        done (will upload an updated patch soon)

        if the index creation time is in the commit data, it will not be removed - as the epoch is added to whatever commit data was read from the index. I think perhaps it should be removed?

        No quite. Every commit, DirTaxoWriter writes a new commitData, combining whatever commitData that is passed from the caller. But it does not merge it with the existing commitData. That's how IndexWriter works too, and it's the responsible of the caller to pass the commitData in every commit(), if he'd like to persist it.
        But, DirTaxoReader does let you read the commitData and so it is possible that someone will obtain the commitData from DirTaxoReader (with the old property), add his stuff to it and pass that to DirTaxoWriter. I don't think that it's critical though, and I doubt if anyone does that.

        ...no test could check the migration from old to new methods...

        Right, I'll add a test case.

        'Epoch' is usually in the context of time

        I don't think that it's critical. Version is problematic since Lucene already uses 'version' and 'generation'. I think that 'epoch' is fine, but if anyone has a better suggestion, I don't mind changing it.

        Show
        Shai Erera added a comment - Thanks for the review. in TestDirectoryTaxonomyWriter.java, the error string "index.create.time not found in commitData" should be updated. done (will upload an updated patch soon) if the index creation time is in the commit data, it will not be removed - as the epoch is added to whatever commit data was read from the index. I think perhaps it should be removed? No quite. Every commit, DirTaxoWriter writes a new commitData, combining whatever commitData that is passed from the caller. But it does not merge it with the existing commitData. That's how IndexWriter works too, and it's the responsible of the caller to pass the commitData in every commit(), if he'd like to persist it. But, DirTaxoReader does let you read the commitData and so it is possible that someone will obtain the commitData from DirTaxoReader (with the old property), add his stuff to it and pass that to DirTaxoWriter. I don't think that it's critical though, and I doubt if anyone does that. ...no test could check the migration from old to new methods... Right, I'll add a test case. 'Epoch' is usually in the context of time I don't think that it's critical. Version is problematic since Lucene already uses 'version' and 'generation'. I think that 'epoch' is fine, but if anyone has a better suggestion, I don't mind changing it.
        Hide
        Shai Erera added a comment -

        ...no test could check the migration from old to new methods...

        Actually there was such test !. TestDirTaxoWriter.testUndefinedCreateTime. I'll rename it to testBackwardsComptability though.

        Show
        Shai Erera added a comment - ...no test could check the migration from old to new methods... Actually there was such test !. TestDirTaxoWriter.testUndefinedCreateTime. I'll rename it to testBackwardsComptability though.
        Hide
        Shai Erera added a comment -

        Patch addresses the comments. For now, I kept the 'epoch' wording, unless there's another suggestion.

        Show
        Shai Erera added a comment - Patch addresses the comments. For now, I kept the 'epoch' wording, unless there's another suggestion.
        Hide
        Shai Erera added a comment -

        Committed to trunk and 4.x. I didn't commit to 4.0.x because it seems we're not going to have a 4.0.1, but rather focus on 4.1.

        Show
        Shai Erera added a comment - Committed to trunk and 4.x. I didn't commit to 4.0.x because it seems we're not going to have a 4.0.1, but rather focus on 4.1.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Shai Erera
        http://svn.apache.org/viewvc?view=revision&revision=1406565

        LUCENE-4532: port from trunk

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Shai Erera http://svn.apache.org/viewvc?view=revision&revision=1406565 LUCENE-4532 : port from trunk

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development