Lucene - Core
  1. Lucene - Core
  2. LUCENE-5945

Full cutover to Path api from java.io.File

    Details

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

      Description

      Using NIO2 has a lot of benefits:

      • more fine grained exception handling
      • clearer semantics about what happens
      • additional functionality
      • possibility to work with virtual filesystems, etc.

      We already banned File.delete and switched to Files.delete, I think we should ban File completely (except for some sugar methods that just forward with .toPath, like FSDirectory.open)

      For tests, ideally we go a little further and ban methods like FileSystems.getDefault(). Instead we could exempt LuceneTestCase and ensure all Paths are created via one protected method. This leaves open the possibility to mock up filesystem behavior at a lower level in tests in the future.

      1. LUCENE-5945_core.patch
        120 kB
        Robert Muir
      2. LUCENE-5945.patch
        468 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          I am currently working on a patch. If anyone else is interested, i can commit it to a branch.

          Show
          Robert Muir added a comment - I am currently working on a patch. If anyone else is interested, i can commit it to a branch.
          Hide
          Dawid Weiss added a comment -

          Pretty cool idea!

          Show
          Dawid Weiss added a comment - Pretty cool idea!
          Hide
          Robert Muir added a comment -

          There is a little complexity, today its not so friendly to any provider:

          • SimpleFSDirectory only works with File since it uses RandomAccessFile, it will not work with any Path (only really File or Path.toFile), because we advertise in docs that you can safely use it without worrying about ClosedByInterruptException.
          • NIOFSDirectory/MMapDirectory require that you can make a FileChannel on the path.

          I hate to add a fourth directory to use Files.newByteChannel just because of the stupid ClosedByInterruptException, and have no idea what it would be named (to me this should be the "Simple" one...)

          Show
          Robert Muir added a comment - There is a little complexity, today its not so friendly to any provider: SimpleFSDirectory only works with File since it uses RandomAccessFile, it will not work with any Path (only really File or Path.toFile), because we advertise in docs that you can safely use it without worrying about ClosedByInterruptException. NIOFSDirectory/MMapDirectory require that you can make a FileChannel on the path. I hate to add a fourth directory to use Files.newByteChannel just because of the stupid ClosedByInterruptException, and have no idea what it would be named (to me this should be the "Simple" one...)
          Hide
          Robert Muir added a comment -

          Here is a start, lucene/core src and tests, and test-framework.

          There are still some uses of File in test-framework and offlinesorter i want to clean up, around temp file handling and so on.

          I didn't look at the other modules yet. I also didn't really add any method for tests to resolve filenames to paths that we could plugin to, because i think we dont need it? really all tests should be using the temporary directory creation mechanism inside LuceneTestCase.

          Anyway the core tests pass (and lock stress tests).

          Show
          Robert Muir added a comment - Here is a start, lucene/core src and tests, and test-framework. There are still some uses of File in test-framework and offlinesorter i want to clean up, around temp file handling and so on. I didn't look at the other modules yet. I also didn't really add any method for tests to resolve filenames to paths that we could plugin to, because i think we dont need it? really all tests should be using the temporary directory creation mechanism inside LuceneTestCase. Anyway the core tests pass (and lock stress tests).
          Hide
          Robert Muir added a comment -

          Also zip file handling isnt fixed yet in TestUtil.

          And TestIndexWriterOnJRECrash spins off a process, so that specific test requires a real file. If we plugin our own stuff we can add some assumption or whatever that its test tempdir.toFile() works. Most tests shouldnt need File.

          Show
          Robert Muir added a comment - Also zip file handling isnt fixed yet in TestUtil. And TestIndexWriterOnJRECrash spins off a process, so that specific test requires a real file. If we plugin our own stuff we can add some assumption or whatever that its test tempdir.toFile() works. Most tests shouldnt need File.
          Hide
          Robert Muir added a comment -

          Here is a more fleshed out patch. All tests pass and java.io.File/FileInputStream/FileOutputStream are banned.

          I think its ready for trunk, we can iterate with more improvements and cleanups after.

          Show
          Robert Muir added a comment - Here is a more fleshed out patch. All tests pass and java.io.File/FileInputStream/FileOutputStream are banned. I think its ready for trunk, we can iterate with more improvements and cleanups after.
          Hide
          Michael McCandless added a comment -

          This looks great, +1! We can iterate if there are issues ...

          Show
          Michael McCandless added a comment - This looks great, +1! We can iterate if there are issues ...
          Hide
          Uwe Schindler added a comment - - edited

          Cool. Commit that shit, except a small improvement:

          One small idea for forbidden-apis: ANT allows it to make parts of a fileset conditionally. So you might make use the isLucene property when building the fileset of signaturesfiles:

          <signaturesfileset dir="...">
          <include name="base.txt"/>
          <include name="base-lucene.txt" if="isLucene"/>
          </signaturesfileset>
          

          This will speed up forbiddenapis, as it does not need to be exceuded multiple times.

          If you have quesions, I might take care tomorrow. I found out that if/unless works with filesets include directive only a few days ago: https://ant.apache.org/manual/Types/patternset.html

          Show
          Uwe Schindler added a comment - - edited Cool. Commit that shit, except a small improvement: One small idea for forbidden-apis: ANT allows it to make parts of a fileset conditionally. So you might make use the isLucene property when building the fileset of signaturesfiles: <signaturesfileset dir= "..." > <include name= "base.txt" /> <include name= "base-lucene.txt" if= "isLucene" /> </signaturesfileset> This will speed up forbiddenapis, as it does not need to be exceuded multiple times. If you have quesions, I might take care tomorrow. I found out that if/unless works with filesets include directive only a few days ago: https://ant.apache.org/manual/Types/patternset.html
          Hide
          Alan Woodward added a comment -

          This looks ace.

          One nit: in FilesystemResourceLoader.java

              throw new IllegalArgumentException("baseDirectory is not a directory")
          

          should be

              throw new IllegalArgumentException(baseDirectory + " is not a directory")
          
          Show
          Alan Woodward added a comment - This looks ace. One nit: in FilesystemResourceLoader.java throw new IllegalArgumentException( "baseDirectory is not a directory" ) should be throw new IllegalArgumentException(baseDirectory + " is not a directory" )
          Hide
          Robert Muir added a comment -

          If you have quesions, I might take care tomorrow. I found out that if/unless works with filesets include directive only a few days ago: https://ant.apache.org/manual/Types/patternset.html

          I will commit this patch and leave that to you. I don't want to refactor the current forbidden APIs logic in this issue. Its already a 500KB patch.

          One nit: in FilesystemResourceLoader.java

          Thank you Alan.

          Show
          Robert Muir added a comment - If you have quesions, I might take care tomorrow. I found out that if/unless works with filesets include directive only a few days ago: https://ant.apache.org/manual/Types/patternset.html I will commit this patch and leave that to you. I don't want to refactor the current forbidden APIs logic in this issue. Its already a 500KB patch. One nit: in FilesystemResourceLoader.java Thank you Alan.
          Hide
          Uwe Schindler added a comment -

          I will commit this patch and leave that to you. I don't want to refactor the current forbidden APIs logic in this issue. Its already a 500KB patch.

          You refactored it! I will fix it tomorrow once you committed (basically revert your additional target). So my suggestion is just to add the signatures file for lucene-only as a conditional to the original signatures fileset instead of adding a new target.

          Show
          Uwe Schindler added a comment - I will commit this patch and leave that to you. I don't want to refactor the current forbidden APIs logic in this issue. Its already a 500KB patch. You refactored it! I will fix it tomorrow once you committed (basically revert your additional target). So my suggestion is just to add the signatures file for lucene-only as a conditional to the original signatures fileset instead of adding a new target.
          Hide
          Robert Muir added a comment -

          I did not refactor it Uwe. There were already 3 or 4 targets, one for each text file. I added one for my new text file.

          I can remove the forbidden apis part of this patch completely when committing. This patch was too much work and I do not care about optimizing 5 microseconds in the build. I care that things work and are correct.

          Show
          Robert Muir added a comment - I did not refactor it Uwe. There were already 3 or 4 targets, one for each text file. I added one for my new text file. I can remove the forbidden apis part of this patch completely when committing. This patch was too much work and I do not care about optimizing 5 microseconds in the build. I care that things work and are correct.
          Hide
          Uwe Schindler added a comment -

          As I said, I will fix the forbidden. Just commit what you have, but don't complain about my changes, I will basically revert just the new target. We already need separate targets, because the base directories for tests and core are different. But we don't need a new target for additional signatures file with conditions. That's already built in.

          Please don't respond so aggressive, I have the right to make suggestions! And I want it correct!

          Show
          Uwe Schindler added a comment - As I said, I will fix the forbidden. Just commit what you have, but don't complain about my changes, I will basically revert just the new target. We already need separate targets, because the base directories for tests and core are different. But we don't need a new target for additional signatures file with conditions. That's already built in. Please don't respond so aggressive, I have the right to make suggestions! And I want it correct!
          Hide
          Uwe Schindler added a comment - - edited

          Basically, thats my change, which is trivial:

           <condition property="isLucene">
              <not>
                <or>
                  <matches pattern="^(solr)\b" string="${name}"/>
                  <matches pattern="tools" string="${name}"/>
                </or>
              </not>
            </condition>
          
            <!-- applies to both source and test code -->
            <target name="-check-forbidden-all" depends="-init-forbidden-apis,compile-core,compile-test">
              <forbidden-apis internalRuntimeForbidden="true" classpathref="forbidden-apis.allclasses.classpath">
                <bundledSignatures name="jdk-unsafe-${javac.target}"/>
                <bundledSignatures name="jdk-deprecated-${javac.target}"/>
                <signaturesFileSet dir="${common.dir}/tools/forbiddenApis">
                  <include name="base.txt"/>
                  <include name="lucene.txt" if="isLucene"/>
                </signaturesFileSet>
                <fileset dir="${build.dir}/classes/java" excludes="${forbidden-base-excludes}"/>
                <fileset dir="${build.dir}/classes/test" excludes="${forbidden-tests-excludes}" erroronmissingdir="false"/>
              </forbidden-apis>
            </target>
          
          Show
          Uwe Schindler added a comment - - edited Basically, thats my change, which is trivial: <condition property= "isLucene" > <not> <or> <matches pattern= "^(solr)\b" string= "${name}" /> <matches pattern= "tools" string= "${name}" /> </or> </not> </condition> <!-- applies to both source and test code --> <target name= "-check-forbidden-all" depends= "-init-forbidden-apis,compile-core,compile-test" > <forbidden-apis internalRuntimeForbidden= "true" classpathref= "forbidden-apis.allclasses.classpath" > <bundledSignatures name= "jdk-unsafe-${javac.target}" /> <bundledSignatures name= "jdk-deprecated-${javac.target}" /> <signaturesFileSet dir= "${common.dir}/tools/forbiddenApis" > <include name= "base.txt" /> <include name= "lucene.txt" if= "isLucene" /> </signaturesFileSet> <fileset dir= "${build.dir}/classes/java" excludes= "${forbidden-base-excludes}" /> <fileset dir= "${build.dir}/classes/test" excludes= "${forbidden-tests-excludes}" erroronmissingdir= "false" /> </forbidden-apis> </target>
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-5945: Full cutover to Path api from java.io.File

          Show
          ASF subversion and git services added a comment - Commit 1624784 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1624784 ] LUCENE-5945 : Full cutover to Path api from java.io.File
          Hide
          Robert Muir added a comment -

          Uwe, please just commit it if it works. I dont understand it myself because my brain is damaged from fixing all file handling in benchmark/

          Show
          Robert Muir added a comment - Uwe, please just commit it if it works. I dont understand it myself because my brain is damaged from fixing all file handling in benchmark/
          Hide
          Robert Muir added a comment -

          I fixed as 5.0. If someone wants to backport, feel free. But I'm focusing on trunk.

          Show
          Robert Muir added a comment - I fixed as 5.0. If someone wants to backport, feel free. But I'm focusing on trunk.
          Hide
          Uwe Schindler added a comment -

          OK, will do this in a minute

          Show
          Uwe Schindler added a comment - OK, will do this in a minute
          Hide
          ASF subversion and git services added a comment -

          Commit 1624785 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1624785 ]

          LUCENE-5945: Changes to forbidden, as discussed on issue.

          Show
          ASF subversion and git services added a comment - Commit 1624785 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1624785 ] LUCENE-5945 : Changes to forbidden, as discussed on issue.
          Hide
          Uwe Schindler added a comment -

          I committed the changes. Basically, the condition would not be needed at all, as Solr already overrides the base checks. But I left the condition in for the tools.

          Show
          Uwe Schindler added a comment - I committed the changes. Basically, the condition would not be needed at all, as Solr already overrides the base checks. But I left the condition in for the tools.
          Hide
          ASF subversion and git services added a comment -

          Commit 1624790 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1624790 ]

          LUCENE-5945: Nuke one more ZipFile usage

          Show
          ASF subversion and git services added a comment - Commit 1624790 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1624790 ] LUCENE-5945 : Nuke one more ZipFile usage
          Hide
          ASF subversion and git services added a comment -

          Commit 1624791 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1624791 ]

          LUCENE-5945: Cleanup & javadocs

          Show
          ASF subversion and git services added a comment - Commit 1624791 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1624791 ] LUCENE-5945 : Cleanup & javadocs
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-5945: more test cleanups

          Show
          ASF subversion and git services added a comment - Commit 1624800 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1624800 ] LUCENE-5945 : more test cleanups
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-5945: fix test bug, ensure 'dir' is always closed

          Show
          ASF subversion and git services added a comment - Commit 1624882 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1624882 ] LUCENE-5945 : fix test bug, ensure 'dir' is always closed
          Hide
          ASF subversion and git services added a comment -

          Commit 1625792 from Michael McCandless in branch 'dev/trunk'
          [ https://svn.apache.org/r1625792 ]

          LUCENE-5945: don't hit NPE on trying to get root's fileName leading to strange test failure

          Show
          ASF subversion and git services added a comment - Commit 1625792 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1625792 ] LUCENE-5945 : don't hit NPE on trying to get root's fileName leading to strange test failure
          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:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development