Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-812

Unable to set LockFactory implementation via ${org.apache.lucene.store.FSDirectoryLockFactoryClass}

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      While trying to move from Lucene 2.0 to Lucene 2.1 I noticed a problem with the LockFactory instantiation code.
      During previous tests we successfully specified the LockFactory implementation by setting the property
      $

      {org.apache.lucene.store.FSDirectoryLockFactoryClass}

      to "org.apache.lucene.store.NativeFSLockFactory".
      This does no longer work due to a bug in the FSDirectory class. The problem is caused from the fact that this
      class tries to invoke the default constructor of the specified LockFactory class. However neither NativeFSLockFactory
      nor SimpleFSLockFactory do have a default constructor.

      FSDirectory, Line 285:
      try

      { lockFactory = (LockFactory) c.newInstance(); }

      catch (IllegalAccessException e)

      { throw new IOException("IllegalAccessException when instantiating LockClass " + lockClassName); }

      catch (InstantiationException e)

      { throw new IOException("InstantiationException when instantiating LockClass " + lockClassName); }

      catch (ClassCastException e)

      { throw new IOException("unable to cast LockClass " + lockClassName + " instance to a LockFactory"); }

      A possible workaround is to not set the property at all and call FSDirectory.setLockFactory(...) instead.

      1. LUCENE-812.patch
        8 kB
        Michael McCandless

        Activity

        Hide
        mikemccand Michael McCandless added a comment -

        The no-argument constructors for Native/SimpleFSLockFactory were
        removed as part of LUCENE-771 (store write.lock in index directory by
        default). These APIs were never released (ie only available in the
        trunk builds).

        Since there is no longer a notion of the "global default lock
        directory", I'm not really sure how to (or whether to) fix this: these
        lock factories really need to know the directory to store their lock
        files in.

        The fallback is to instantiate the LockFactory ahead of time and pass
        it into FSDirectory.getDirectory(). Is that fallback workable here?

        I'd lean towards "won't fix" on this unless someone can suggest a good
        solution. That property can still be used for other lock factory
        instances that require no arguments on construction.

        Show
        mikemccand Michael McCandless added a comment - The no-argument constructors for Native/SimpleFSLockFactory were removed as part of LUCENE-771 (store write.lock in index directory by default). These APIs were never released (ie only available in the trunk builds). Since there is no longer a notion of the "global default lock directory", I'm not really sure how to (or whether to) fix this: these lock factories really need to know the directory to store their lock files in. The fallback is to instantiate the LockFactory ahead of time and pass it into FSDirectory.getDirectory(). Is that fallback workable here? I'd lean towards "won't fix" on this unless someone can suggest a good solution. That property can still be used for other lock factory instances that require no arguments on construction.
        Hide
        hossman Hoss Man added a comment -

        I'm not sure if i understand what the bug is ... as far as i can tell, LockFactory didnt' exist in 2.0.

        (Since we've been actively moving away from specifying class instances with System Properties, i'm suprised to even see code in 2.1 that allows the LockFactory to be specified in that way.)

        Show
        hossman Hoss Man added a comment - I'm not sure if i understand what the bug is ... as far as i can tell, LockFactory didnt' exist in 2.0. (Since we've been actively moving away from specifying class instances with System Properties, i'm suprised to even see code in 2.1 that allows the LockFactory to be specified in that way.)
        Hide
        mikemccand Michael McCandless added a comment -

        Actually it was your response to the original proposal for the LockFactory changes that caused me to leave the system property path in there

        http://www.gossamer-threads.com/lists/lucene/java-dev/37401#37401

        I think having both options can be helpful, but, system properties are dangerous because they are global so perhaps going forward we should try not add any new system properties...

        Show
        mikemccand Michael McCandless added a comment - Actually it was your response to the original proposal for the LockFactory changes that caused me to leave the system property path in there http://www.gossamer-threads.com/lists/lucene/java-dev/37401#37401 I think having both options can be helpful, but, system properties are dangerous because they are global so perhaps going forward we should try not add any new system properties...
        Hide
        hossman Hoss Man added a comment -

        > Actually it was your response to the original proposal for the LockFactory changes that caused me to
        > leave the system property path in there

        really ?... oh man, i feel silly – I think I was refering to the making sure that long standing "org.apache.lucene.FSDirectory.class" system property still worked for backwards compatibility ... but June 2006 was a long time ago, who knows what i was thinking

        having a system property for the LockFactory is al fine and dandy, just as long as people realize it doesn't work with most of the out-of-the-box LockFactories since they have constructor args.

        if we wanted to be really aggresive about supporting system props for things like this, there are probably things we could do to make it work better – but i don't think we should aggresivly support using system props to set things like this. as long as we have setters for things, client code can use system properties all it wants to set these things.

        Show
        hossman Hoss Man added a comment - > Actually it was your response to the original proposal for the LockFactory changes that caused me to > leave the system property path in there really ?... oh man, i feel silly – I think I was refering to the making sure that long standing "org.apache.lucene.FSDirectory.class" system property still worked for backwards compatibility ... but June 2006 was a long time ago, who knows what i was thinking having a system property for the LockFactory is al fine and dandy, just as long as people realize it doesn't work with most of the out-of-the-box LockFactories since they have constructor args. if we wanted to be really aggresive about supporting system props for things like this, there are probably things we could do to make it work better – but i don't think we should aggresivly support using system props to set things like this. as long as we have setters for things, client code can use system properties all it wants to set these things.
        Hide
        make Matthias Kerkhoff added a comment -

        From the perspective of Lucene user I would like to add ...

        • if you leave the code in there, it should work (which should be doable, the missing argument is the lock directory, which defaults to the index directory or, if present, from $org.apache.lucene.lockdir). Currently there is not a single LockFactory that will NOT cause an InstantiationException.
        • if the code is a leftover from some intermediate nightly snapshot, remove it and remove any reference to the system property too.

        I can live with both scenarios. I just entered this issue as I thought that the behaviour of the code ... is a bit unexpected. In the end, if the system property is set to whatever value lucene is unable to create an FSDirectory and FSDirectory based IndexSearchers.

        Show
        make Matthias Kerkhoff added a comment - From the perspective of Lucene user I would like to add ... if you leave the code in there, it should work (which should be doable, the missing argument is the lock directory, which defaults to the index directory or, if present, from $org.apache.lucene.lockdir). Currently there is not a single LockFactory that will NOT cause an InstantiationException. if the code is a leftover from some intermediate nightly snapshot, remove it and remove any reference to the system property too. I can live with both scenarios. I just entered this issue as I thought that the behaviour of the code ... is a bit unexpected. In the end, if the system property is set to whatever value lucene is unable to create an FSDirectory and FSDirectory based IndexSearchers.
        Hide
        mikemccand Michael McCandless added a comment -

        > I just entered this issue as I thought that the behaviour of the
        > code ... is a bit unexpected. In the end, if the system property is
        > set to whatever value lucene is unable to create an FSDirectory and
        > FSDirectory based IndexSearchers.

        Agreed, this really is a bug (thanks for finding it & opening it
        Matthias!).

        > ... but i don't think we should aggresivly support using system
        > props to set things like this. as long as we have setters for
        > things, client code can use system properties all it wants to set
        > these things.

        +1

        I think system properties are dangerously global and we should try not
        to add any more to Lucene?

        I would prefer to just remove this one (and do a 2.1.1 soon?) or
        deprecate this property for the next release.

        Show
        mikemccand Michael McCandless added a comment - > I just entered this issue as I thought that the behaviour of the > code ... is a bit unexpected. In the end, if the system property is > set to whatever value lucene is unable to create an FSDirectory and > FSDirectory based IndexSearchers. Agreed, this really is a bug (thanks for finding it & opening it Matthias!). > ... but i don't think we should aggresivly support using system > props to set things like this. as long as we have setters for > things, client code can use system properties all it wants to set > these things. +1 I think system properties are dangerously global and we should try not to add any more to Lucene? I would prefer to just remove this one (and do a 2.1.1 soon?) or deprecate this property for the next release.
        Hide
        mikemccand Michael McCandless added a comment -

        One of the original motivations of this property was the ability to
        also choose LockFactory implementations external to Lucene (the
        original issue had a lock factory that used MySQL for locking). So
        the property is still useful for that use-case.

        Unfortunately we can't deprecate this (there is no compiler/runtime
        support for deprecating a random property?). I guess we could remove
        support for it (in 2.2 or 2.1.1?) and throw an exception if we see
        that the property is set (to catch users who had started using it) but
        that seems rather harsh.

        So ... I think we should in fact support it (but in the future we
        should not add any more new System properties to Lucene, I think).

        I plan to modify Simple/NativeFSLockFactory to have no arg constructor
        that sets their lockDir to null, and then add logic to
        FSDirectory.getDirectory to set its own dir as the lockDirName if it
        sees that the incoming LockFactory is one of these two instances with
        a null lockDir.

        The other 2 builtin LockFactory implementations already have no-arg
        constructors so they work fine through this property.

        Show
        mikemccand Michael McCandless added a comment - One of the original motivations of this property was the ability to also choose LockFactory implementations external to Lucene (the original issue had a lock factory that used MySQL for locking). So the property is still useful for that use-case. Unfortunately we can't deprecate this (there is no compiler/runtime support for deprecating a random property?). I guess we could remove support for it (in 2.2 or 2.1.1?) and throw an exception if we see that the property is set (to catch users who had started using it) but that seems rather harsh. So ... I think we should in fact support it (but in the future we should not add any more new System properties to Lucene, I think). I plan to modify Simple/NativeFSLockFactory to have no arg constructor that sets their lockDir to null, and then add logic to FSDirectory.getDirectory to set its own dir as the lockDirName if it sees that the incoming LockFactory is one of these two instances with a null lockDir. The other 2 builtin LockFactory implementations already have no-arg constructors so they work fine through this property.
        Hide
        mikemccand Michael McCandless added a comment -

        Attached proposed patch.

        OK I made the no-argument constructors package-private since you would
        not normally do this. I also added package-private "setLockDir()".
        FSDirectory uses setLockDir to set itself when it instantiates a
        Simple/NativeFSLockFactory using that System property. I also
        extended TestLockFactory test case to test that all 4 builtin
        LockFactory implementations can be set.

        Show
        mikemccand Michael McCandless added a comment - Attached proposed patch. OK I made the no-argument constructors package-private since you would not normally do this. I also added package-private "setLockDir()". FSDirectory uses setLockDir to set itself when it instantiates a Simple/NativeFSLockFactory using that System property. I also extended TestLockFactory test case to test that all 4 builtin LockFactory implementations can be set.

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            make Matthias Kerkhoff
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development