Solr
  1. Solr
  2. SOLR-6519

In trunk change Solr's DirectoryFactory.create method to take LockFactory

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Because of NIO2 changes and the corresponding workaround, it is impossible now to create a Directory and "hope" that the lock factory directory is not created. Especially if you want some non-standard lock factory, this blows up.

      The problem is: The lock dir is now created in ctor. As workaround I made all factories set NoLockFactory initially through ctor (see SOLR-6518), but this is just a workaround for incorrect API design.

      In fact the main problem is just stupid: Why does protected CachingDirectoryFactory.create() not take the lock factory? I think its because of backwards compatibility, but with Solr 5.0 we can change this.

      In future we want to make the lock factory non-mutable in Directory, so this is an important change. In addition, injectLockFactory looks horrible, this code is a häckidy-hick-hack!

      1. SOLR-6519.patch
        30 kB
        Uwe Schindler
      2. SOLR-6519.patch
        25 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Attached is a patch refactoring DirectoryFactory in Solr 5.0:

          • injectLockFactory was removed, it was a very ugly hack
          • the abstract create() method now takes a LockFactory instance, that is used to pass to ctor. Some directory implementations may ignore it.
          • I added createLockFactory(9 protected method, that has a default implementation for standard file systems lock factories. It only does not allow HdfsLockFactory, instead throws an Exception. HdfsDirectory ofverrides this method and allows to create one with correct configuration. RAMDirectoryFactory only allows "single" as lock factory, other settings are refused.

          Just one Question: If you don't configure a lock type, it currently falls back to "simple" as default (for backwards compatibility). For Solr 5, we should change this default to "native". Any comments?

          Mark Miller: Can you look at the HDFS stuff, I was not able to test this on Windows.

          All tests pass.

          Show
          Uwe Schindler added a comment - Attached is a patch refactoring DirectoryFactory in Solr 5.0: injectLockFactory was removed, it was a very ugly hack the abstract create() method now takes a LockFactory instance, that is used to pass to ctor. Some directory implementations may ignore it. I added createLockFactory(9 protected method, that has a default implementation for standard file systems lock factories. It only does not allow HdfsLockFactory, instead throws an Exception. HdfsDirectory ofverrides this method and allows to create one with correct configuration. RAMDirectoryFactory only allows "single" as lock factory, other settings are refused. Just one Question: If you don't configure a lock type, it currently falls back to "simple" as default (for backwards compatibility). For Solr 5, we should change this default to "native". Any comments? Mark Miller : Can you look at the HDFS stuff, I was not able to test this on Windows. All tests pass.
          Hide
          Uwe Schindler added a comment -

          New patch with more refactorings...

          All test pass. I changed the default lockfactory for FS-based directories to "native".

          Show
          Uwe Schindler added a comment - New patch with more refactorings... All test pass. I changed the default lockfactory for FS-based directories to "native".
          Hide
          Uwe Schindler added a comment -

          New patch: The last one missed to set the lock factory on HDFSDirectory.

          It's ready to commit.

          Show
          Uwe Schindler added a comment - New patch: The last one missed to set the lock factory on HDFSDirectory. It's ready to commit.
          Hide
          ASF subversion and git services added a comment -

          Commit 1625644 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1625644 ]

          SOLR-6519: Make DirectoryFactory#create() take LockFactory

          Show
          ASF subversion and git services added a comment - Commit 1625644 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1625644 ] SOLR-6519 : Make DirectoryFactory#create() take LockFactory
          Hide
          Hoss Man added a comment -

          I'm seeing reproducible test failures on trunk that seem related to this change...

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=CoreMergeIndexesAdminHandlerTest -Dtests.method=testMergeIndexesCoreAdminHandler -Dtests.seed=A61E329DCBF0FF41 -Dtests.slow=true -Dtests.locale=pt -Dtests.timezone=Pacific/Pago_Pago -Dtests.file.encoding=UTF-8
             [junit4] ERROR   0.05s | CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler <<<
             [junit4]    > Throwable #1: org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: Unrecognized lockType: single
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([A61E329DCBF0FF41:6374CDE1159B9BFA]:0)
             [junit4]    > 	at org.apache.solr.core.CoreContainer.getCore(CoreContainer.java:745)
             [junit4]    > 	at org.apache.solr.handler.admin.CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler(CoreMergeIndexesAdminHandlerTest.java:81)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
             [junit4]    > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single
             [junit4]    > 	at org.apache.solr.core.SolrCore.<init>(SolrCore.java:889)
             [junit4]    > 	at org.apache.solr.core.SolrCore.<init>(SolrCore.java:651)
          
          Show
          Hoss Man added a comment - I'm seeing reproducible test failures on trunk that seem related to this change... [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=CoreMergeIndexesAdminHandlerTest -Dtests.method=testMergeIndexesCoreAdminHandler -Dtests.seed=A61E329DCBF0FF41 -Dtests.slow=true -Dtests.locale=pt -Dtests.timezone=Pacific/Pago_Pago -Dtests.file.encoding=UTF-8 [junit4] ERROR 0.05s | CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler <<< [junit4] > Throwable #1: org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: Unrecognized lockType: single [junit4] > at __randomizedtesting.SeedInfo.seed([A61E329DCBF0FF41:6374CDE1159B9BFA]:0) [junit4] > at org.apache.solr.core.CoreContainer.getCore(CoreContainer.java:745) [junit4] > at org.apache.solr.handler.admin.CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler(CoreMergeIndexesAdminHandlerTest.java:81) [junit4] > at java.lang.Thread.run(Thread.java:745) [junit4] > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single [junit4] > at org.apache.solr.core.SolrCore.<init>(SolrCore.java:889) [junit4] > at org.apache.solr.core.SolrCore.<init>(SolrCore.java:651)
          Hide
          Hoss Man added a comment -

          another seed, full stack...

             [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=CoreMergeIndexesAdminHandlerTest -Dtests.method=testMergeIndexesCoreAdminHandler -Dtests.seed=CD7BE4551EE0F637 -Dtests.slow=true -Dtests.locale=zh_SG -Dtests.timezone=Asia/Calcutta -Dtests.file.encoding=US-ASCII
             [junit4] ERROR   0.05s | CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler <<<
             [junit4]    > Throwable #1: org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: Unrecognized lockType: single
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([CD7BE4551EE0F637:8111B29C08B928C]:0)
             [junit4]    > 	at org.apache.solr.core.CoreContainer.getCore(CoreContainer.java:745)
             [junit4]    > 	at org.apache.solr.handler.admin.CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler(CoreMergeIndexesAdminHandlerTest.java:81)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
             [junit4]    > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single
             [junit4]    > 	at org.apache.solr.core.SolrCore.<init>(SolrCore.java:889)
             [junit4]    > 	at org.apache.solr.core.SolrCore.<init>(SolrCore.java:651)
             [junit4]    > 	at org.apache.solr.core.CoreContainer.create(CoreContainer.java:491)
             [junit4]    > 	at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:255)
             [junit4]    > 	at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:249)
             [junit4]    > 	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
             [junit4]    > 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
             [junit4]    > 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
             [junit4]    > 	... 1 more
             [junit4]    > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single
             [junit4]    > 	at org.apache.solr.core.StandardDirectoryFactory.createLockFactory(StandardDirectoryFactory.java:73)
             [junit4]    > 	at org.apache.solr.core.CachingDirectoryFactory.get(CachingDirectoryFactory.java:350)
             [junit4]    > 	at org.apache.solr.core.SolrCore.getNewIndexDir(SolrCore.java:275)
             [junit4]    > 	at org.apache.solr.core.SolrCore.initIndex(SolrCore.java:487)
             [junit4]    > 	at org.apache.solr.core.SolrCore.<init>(SolrCore.java:793)
             [junit4]    > 	... 8 more
          
          Show
          Hoss Man added a comment - another seed, full stack... [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=CoreMergeIndexesAdminHandlerTest -Dtests.method=testMergeIndexesCoreAdminHandler -Dtests.seed=CD7BE4551EE0F637 -Dtests.slow=true -Dtests.locale=zh_SG -Dtests.timezone=Asia/Calcutta -Dtests.file.encoding=US-ASCII [junit4] ERROR 0.05s | CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler <<< [junit4] > Throwable #1: org.apache.solr.common.SolrException: SolrCore 'collection1' is not available due to init failure: Unrecognized lockType: single [junit4] > at __randomizedtesting.SeedInfo.seed([CD7BE4551EE0F637:8111B29C08B928C]:0) [junit4] > at org.apache.solr.core.CoreContainer.getCore(CoreContainer.java:745) [junit4] > at org.apache.solr.handler.admin.CoreMergeIndexesAdminHandlerTest.testMergeIndexesCoreAdminHandler(CoreMergeIndexesAdminHandlerTest.java:81) [junit4] > at java.lang.Thread.run(Thread.java:745) [junit4] > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single [junit4] > at org.apache.solr.core.SolrCore.<init>(SolrCore.java:889) [junit4] > at org.apache.solr.core.SolrCore.<init>(SolrCore.java:651) [junit4] > at org.apache.solr.core.CoreContainer.create(CoreContainer.java:491) [junit4] > at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:255) [junit4] > at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:249) [junit4] > at java.util.concurrent.FutureTask.run(FutureTask.java:262) [junit4] > at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) [junit4] > at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) [junit4] > ... 1 more [junit4] > Caused by: org.apache.solr.common.SolrException: Unrecognized lockType: single [junit4] > at org.apache.solr.core.StandardDirectoryFactory.createLockFactory(StandardDirectoryFactory.java:73) [junit4] > at org.apache.solr.core.CachingDirectoryFactory.get(CachingDirectoryFactory.java:350) [junit4] > at org.apache.solr.core.SolrCore.getNewIndexDir(SolrCore.java:275) [junit4] > at org.apache.solr.core.SolrCore.initIndex(SolrCore.java:487) [junit4] > at org.apache.solr.core.SolrCore.<init>(SolrCore.java:793) [junit4] > ... 8 more
          Hide
          Uwe Schindler added a comment -

          Hi,
          I know why this happens:
          In the new impl (after removing the hacky) injectLockFactory, I only added the "useful" ones to each DirectoryFactory:

          • HDFS only supports "none" or "hdfs"
          • StandardDirectoryFactory only supports "simple", "native", "none"
          • RAMDirectoryFactory only supports "single"
          • MockDirectoryFactory ignores the factory completely, it uses what test framework chooses

          I did not know that there was some additional randomization! For me the tests passed - strange. We have 2 opinions:

          • Add back "single" to StandardDirectoryFactory (which is a bit strange), but Hoss mentioned on IRC, if somebody has a separate directory for every solr instance. It is still risky!
          • Change the randomization
          Show
          Uwe Schindler added a comment - Hi, I know why this happens: In the new impl (after removing the hacky) injectLockFactory, I only added the "useful" ones to each DirectoryFactory: HDFS only supports "none" or "hdfs" StandardDirectoryFactory only supports "simple", "native", "none" RAMDirectoryFactory only supports "single" MockDirectoryFactory ignores the factory completely, it uses what test framework chooses I did not know that there was some additional randomization! For me the tests passed - strange. We have 2 opinions: Add back "single" to StandardDirectoryFactory (which is a bit strange), but Hoss mentioned on IRC, if somebody has a separate directory for every solr instance. It is still risky! Change the randomization
          Hide
          ASF subversion and git services added a comment -

          Commit 1625724 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1625724 ]

          SOLR-6519: Add back "single" lock factory type to StandardDirectoryFactory (to prevent test randomization from failing). Should be discussed in separate issue.

          Show
          ASF subversion and git services added a comment - Commit 1625724 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1625724 ] SOLR-6519 : Add back "single" lock factory type to StandardDirectoryFactory (to prevent test randomization from failing). Should be discussed in separate issue.
          Hide
          Uwe Schindler added a comment -

          For now I will support "single" again for StandardDirectoryFactory, so tests don't fail. Its a risky setting, but if we want to keep it for now, I am fine.

          I committed this. I will open anther issue to circulate about "single" vs. disk based directory.

          Show
          Uwe Schindler added a comment - For now I will support "single" again for StandardDirectoryFactory, so tests don't fail. Its a risky setting, but if we want to keep it for now, I am fine. I committed this. I will open anther issue to circulate about "single" vs. disk based directory.
          Hide
          ASF subversion and git services added a comment -

          Commit 1625731 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1625731 ]

          SOLR-6519: Also allow "single" for HDFS

          Show
          ASF subversion and git services added a comment - Commit 1625731 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1625731 ] SOLR-6519 : Also allow "single" for HDFS
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development