Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major 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
          Robert Muir added a comment -

          initial patch.

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

          +1

          Show
          Adrien Grand added a comment - +1
          Hide
          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
          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
          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
          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
          Uwe Schindler added a comment -

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

          Show
          Uwe Schindler added a comment - I opened LUCENE-6150 to remove the broken staleFiles map in FSDirectory.
          Hide
          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
          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
          Uwe Schindler added a comment -

          Not a chance.

          OK

          Show
          Uwe Schindler added a comment - Not a chance. OK
          Hide
          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
          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
          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
          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
          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
          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
          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:
              Unassigned
              Reporter:
              Robert Muir
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development