Lucene - Core
  1. Lucene - Core
  2. LUCENE-2689

remove NativeFSLockFactory's attempt to acquire a test lock

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.1
    • Fix Version/s: 2.9.4, 3.0.3, 3.1, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      NativeFSLockFactory tries to acquire a test lock the first time a lock is created. It's the only LF to do this, and, it's caused us hassle (LUCENE-2421, LUCENE-2688).

      I think we should just remove it. The caller of .makeLock will presumably immediately thereafter acquire the lock and at the point hit any exception that acquireTestLock would've hit.

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          All tests passed for me.

          BTW, according to the code, this method was added due to NFS cache issues, but I agree that it's useless to call makeLock just to call obtain() some time later on. It's almost as if we should make makeLock obtain the lock right away. Is there a reason why we don't do it? Is it essential to separate it into two calls?

          Show
          Shai Erera added a comment - All tests passed for me. BTW, according to the code, this method was added due to NFS cache issues, but I agree that it's useless to call makeLock just to call obtain() some time later on. It's almost as if we should make makeLock obtain the lock right away. Is there a reason why we don't do it? Is it essential to separate it into two calls?
          Hide
          Michael McCandless added a comment -

          BTW, according to the code, this method was added due to NFS cache issues,

          Yeah, but, we now recommend SimpleFSLockFactory for NFS... so I think it's OK to remove this, now.

          I think the makeLock/obtain/release separation is OK? Ie a Lock is stateful (eg can have an open file) so it makes sense that we return a real object...

          Show
          Michael McCandless added a comment - BTW, according to the code, this method was added due to NFS cache issues, Yeah, but, we now recommend SimpleFSLockFactory for NFS... so I think it's OK to remove this, now. I think the makeLock/obtain/release separation is OK? Ie a Lock is stateful (eg can have an open file) so it makes sense that we return a real object...
          Hide
          Shai Erera added a comment -

          Oh sure, we should return an Object upon makeLock. I was just asking whether makeLock should return an already obtained lock. Instead of just "return new Lock()", we do "Lock l = new Lock(); l.obtain();". That way, if obtain fails, makeLock fails as well. That's sort of what acquireTestLock attempted to achieve, no?

          At any rate, today you can call makeLock, succeed in obtaining the test lock, but later fail on obtaining the real lock. Hence, if we attempt to obtain on makeLock (removing obtain from the API?), we guarantee to the user that this lock is valid and should later be released.

          I don't mind if we stay w/ the current API as well.

          Show
          Shai Erera added a comment - Oh sure, we should return an Object upon makeLock. I was just asking whether makeLock should return an already obtained lock. Instead of just "return new Lock()", we do "Lock l = new Lock(); l.obtain();". That way, if obtain fails, makeLock fails as well. That's sort of what acquireTestLock attempted to achieve, no? At any rate, today you can call makeLock, succeed in obtaining the test lock, but later fail on obtaining the real lock. Hence, if we attempt to obtain on makeLock (removing obtain from the API?), we guarantee to the user that this lock is valid and should later be released. I don't mind if we stay w/ the current API as well.
          Hide
          Shai Erera added a comment -

          Mike - are you going to commit this?

          BTW, shouldn't we have handled it as part of LUCENE-2688? One of them is a duplicate of the other I think?

          Show
          Shai Erera added a comment - Mike - are you going to commit this? BTW, shouldn't we have handled it as part of LUCENE-2688 ? One of them is a duplicate of the other I think?
          Hide
          Michael McCandless added a comment -

          Sorry yeah I'll commit this shortly.

          I'll resolve LUCENE-2688 after I resolve this one...

          Show
          Michael McCandless added a comment - Sorry yeah I'll commit this shortly. I'll resolve LUCENE-2688 after I resolve this one...

            People

            • Assignee:
              Michael McCandless
              Reporter:
              Michael McCandless
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development