Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 5.0
    • Component/s: modules/test-framework
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Currently MockDirWrapper always calls delegate.sync() with a comment that we can relax this to not wear out the hardware for tests. The issue, as discussed on this thread http://lucene.markmail.org/thread/eozdsbdahzhjvizj, is related to NRTCachingDirectory and RateLimiter. The improvements I'd like to make under this issue are:

      • Call delgeate.sync() if:
        • rarely()
        • delegate is NRTCachingDir
        • delegate is RateLimitedDirWrapper and its delegate is NRTCachingDir
        • delegate is TrackingDirWrapper and its delegate is NRTCachingDir
      • Also, today the method either fails to sync all files or succeeds. Rather, we can improve this to randomly throw IOE on each file.

      Any other Directories that can cause issues when sync() isn't called?

      1. LUCENE-4990.patch
        4 kB
        Shai Erera
      2. LUCENE-4990.patch
        4 kB
        Shai Erera
      3. LUCENE-4990.patch
        4 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Patch with the mentioned fixes. Note that now I don't remove all names from unsynced files, only the ones that successfully synced.

        Show
        Shai Erera added a comment - Patch with the mentioned fixes. Note that now I don't remove all names from unsynced files, only the ones that successfully synced.
        Hide
        Michael McCandless added a comment -

        +1

        But, I rather liked the comment "corrumpt..."

        Also, can you factor out that large instanceof if into a separate method, eg named "mustSync" or something, with a comment explaining that certain dir impls require sync'ing? If we add future wrapping dir impls we need to update this method ...

        Show
        Michael McCandless added a comment - +1 But, I rather liked the comment "corrumpt..." Also, can you factor out that large instanceof if into a separate method, eg named "mustSync" or something, with a comment explaining that certain dir impls require sync'ing? If we add future wrapping dir impls we need to update this method ...
        Hide
        Shai Erera added a comment -

        Patch factors out a mustSync() method.

        Show
        Shai Erera added a comment - Patch factors out a mustSync() method.
        Hide
        Michael McCandless added a comment -

        +1, thanks Shai!

        Show
        Michael McCandless added a comment - +1, thanks Shai!
        Hide
        Robert Muir added a comment -

        Thanks for tackling this!

        as far as mustSync(), maybe it should be a loop?
        something like:

        
        while (delegate instanceof RateLimited || delegate instanceof Tracking) {
          delegate = xxx.getDelegate();
        }
        

        It just isnt obvious from our test code if we create complex situations like Tracking(RateLimited(NRT))), and i know that when tracking was first added, it didnt immediately cause tons of tests to fail, unfortunately.

        Show
        Robert Muir added a comment - Thanks for tackling this! as far as mustSync(), maybe it should be a loop? something like: while (delegate instanceof RateLimited || delegate instanceof Tracking) { delegate = xxx.getDelegate(); } It just isnt obvious from our test code if we create complex situations like Tracking(RateLimited(NRT))), and i know that when tracking was first added, it didnt immediately cause tons of tests to fail, unfortunately.
        Hide
        Shai Erera added a comment -

        Good idea Rob! Patch checks delegate in a loop. The loop is rather weird though, would have been easier if both dirs implemented a DelegateDirectory (or FilterDirectory), but that's what we have for now.

        Show
        Shai Erera added a comment - Good idea Rob! Patch checks delegate in a loop. The loop is rather weird though, would have been easier if both dirs implemented a DelegateDirectory (or FilterDirectory), but that's what we have for now.
        Hide
        Robert Muir added a comment -

        +1

        Show
        Robert Muir added a comment - +1
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] shaie
        http://svn.apache.org/viewvc?view=revision&revision=1480761

        LUCENE-4990: Improve MockDirectoryWrapper.sync

        Show
        Commit Tag Bot added a comment - [trunk commit] shaie http://svn.apache.org/viewvc?view=revision&revision=1480761 LUCENE-4990 : Improve MockDirectoryWrapper.sync
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] shaie
        http://svn.apache.org/viewvc?view=revision&revision=1480764

        LUCENE-4990: Improve MockDirectoryWrapper.sync

        Show
        Commit Tag Bot added a comment - [branch_4x commit] shaie http://svn.apache.org/viewvc?view=revision&revision=1480764 LUCENE-4990 : Improve MockDirectoryWrapper.sync
        Hide
        Shai Erera added a comment -

        Thanks for the review guys. Committed to trunk and 4x.

        Show
        Shai Erera added a comment - Thanks for the review guys. Committed to trunk and 4x.
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development