Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff of LUCENE-4746.

      This method is currently:

      copy(Directory to, String src, String dest, IOContext context)
      

      But it would be better to restructure this so the destination directory is the one actually being changed by the operation:

      copyFrom(Directory from, String src, String dest, IOContext context)
      

      Besides fixing the order to make sense, adding it to the name might help prevent bugs like the current TrackingDirectoryWrapper impl (used by IndexWriter to track what files are used):

      public void copy(Directory to, String src, String dest, IOContext context) throws IOException {
        createdFileNames.add(dest); // BUG!
        in.copy(to, src, dest, context);
      }
      

        Issue Links

          Activity

          Hide
          rcmuir Robert Muir added a comment -

          initial patch.

          Show
          rcmuir Robert Muir added a comment - initial patch.
          Hide
          jpountz Adrien Grand added a comment -

          +1

          Show
          jpountz Adrien Grand added a comment - +1
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          +1

          Hi, this confused me yesterday when responding to the java-user ML request yesterday. Indeed, the target directory should have the copy option, because it is modified.

          Thanks for finding this horrible bug in TrackingDirectoryWrapper!

          One question: I did not know that we already have this tracking wrapper, could we not use it to have the same effect like the horrible "staleFiles" horror in FSDirectory? We could remove it from FSDirectory and let IndexWriter track itsself which files have to be synced?

          Show
          thetaphi Uwe Schindler added a comment - - edited +1 Hi, this confused me yesterday when responding to the java-user ML request yesterday. Indeed, the target directory should have the copy option, because it is modified. Thanks for finding this horrible bug in TrackingDirectoryWrapper! One question: I did not know that we already have this tracking wrapper, could we not use it to have the same effect like the horrible "staleFiles" horror in FSDirectory? We could remove it from FSDirectory and let IndexWriter track itsself which files have to be synced?
          Hide
          thetaphi Uwe Schindler added a comment -

          Ah, don't forget to add a note into MIGRATE.txt of Lucene 5! This may affect users who implement own directories where their optimization may suddenly no longer used (and they have not used @Override annotation, so compiler does not catch the bug).

          Show
          thetaphi Uwe Schindler added a comment - Ah, don't forget to add a note into MIGRATE.txt of Lucene 5! This may affect users who implement own directories where their optimization may suddenly no longer used (and they have not used @Override annotation, so compiler does not catch the bug).
          Hide
          thetaphi Uwe Schindler added a comment -

          I opened LUCENE-6150 to remove the broken staleFiles map in FSDirectory.

          Show
          thetaphi Uwe Schindler added a comment - I opened LUCENE-6150 to remove the broken staleFiles map in FSDirectory.
          Hide
          rcmuir Robert Muir added a comment -

          Ah, don't forget to add a note into MIGRATE.txt of Lucene 5! This may affect users who implement own directories where their optimization may suddenly no longer used (and they have not used @Override annotation, so compiler does not catch the bug).

          Not a chance. Such cases will still work correctly, and thats the users fault. We don't need to clutter up migration notes with unnecessary shit like that.

          Show
          rcmuir Robert Muir added a comment - Ah, don't forget to add a note into MIGRATE.txt of Lucene 5! This may affect users who implement own directories where their optimization may suddenly no longer used (and they have not used @Override annotation, so compiler does not catch the bug). Not a chance. Such cases will still work correctly, and thats the users fault. We don't need to clutter up migration notes with unnecessary shit like that.
          Hide
          thetaphi Uwe Schindler added a comment -

          Not a chance.

          OK

          Show
          thetaphi Uwe Schindler added a comment - Not a chance. OK
          Hide
          rcmuir Robert Muir added a comment -

          Sorry, i am just unhappy thinking about optimizations when we have these bugs in the directory API.

          I am ok with a one-liner to warn there. But honestly this method is still not going to work well for "subclassing" anyway. Its almost pointless to optimize the impls because something like a FilterDirectory will easily wipe away the optimization.

          An alternative solution would be a method on Directory to optionally return a Path for a filename, or some abstraction like a "handle", but i hate overengineering it.

          Show
          rcmuir Robert Muir added a comment - Sorry, i am just unhappy thinking about optimizations when we have these bugs in the directory API. I am ok with a one-liner to warn there. But honestly this method is still not going to work well for "subclassing" anyway. Its almost pointless to optimize the impls because something like a FilterDirectory will easily wipe away the optimization. An alternative solution would be a method on Directory to optionally return a Path for a filename, or some abstraction like a "handle", but i hate overengineering it.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1648851 from Robert Muir in branch 'dev/trunk'
          [ https://svn.apache.org/r1648851 ]

          LUCENE-6146: Replaced Directory.copy() with Directory.copyFrom()

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1648851 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1648851 ] LUCENE-6146 : Replaced Directory.copy() with Directory.copyFrom()
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1648854 from Robert Muir in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1648854 ]

          LUCENE-6146: Replaced Directory.copy() with Directory.copyFrom()

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1648854 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1648854 ] LUCENE-6146 : Replaced Directory.copy() with Directory.copyFrom()
          Hide
          anshumg Anshum Gupta added a comment -

          Bulk close after 5.0 release.

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

            People

            • Assignee:
              Unassigned
              Reporter:
              rcmuir Robert Muir
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development