Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Mon, 4 Jul 2022 19:30:50 GMT, Alan Bateman wrote: > The updated TestDelegation test is looking a bit better now but I think it > would be simplified a lot more by getting rid of the data providers, just > aren't needed in this test. Unless you feel this is a must, I would prefer to keep the DataProviders. The benefit I see is the test code does not need to be duplicated per parameter, each test scenario can be run as an individual test so that you do not need extra plumbing to run each test scenario in the unlikely event of a failure. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Tue, 5 Jul 2022 09:54:15 GMT, Lance Andersen wrote: > Unless you feel this is a must, I would prefer to keep the DataProviders. The > benefit I see is the test code does not need to be duplicated per parameter, > each test scenario can be run as an individual test so that you do not need > extra plumbing to run each test scenario in the unlikely event of a failure. Okay, but there are a few other things to mention: One issue is the reset method is called at the end of each test. I think it needs to be at the beginning of the method, moved to a finally block of a try-finally, or maybe @BeforeMethod to reset before each test. The reason is that one test failing will cause the tests that follow to fail too. The fields aren't constants so looks a bit strange (to me anyway) to be in uppercase. If you rename them then I think the tests would be a bit more readable. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v7]
On Tue, 5 Jul 2022 19:17:27 GMT, Lance Andersen wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Add @BeforeMethod and rename fields I think all my comments have been addressed so I think this is good to go. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v7]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add @BeforeMethod and rename fields - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/240c38b8..74436951 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=06 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=05-06 Stats: 36 lines in 1 file changed: 9 ins; 3 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Tue, 5 Jul 2022 10:56:45 GMT, Alan Bateman wrote: > > Unless you feel this is a must, I would prefer to keep the DataProviders. > > The benefit I see is the test code does not need to be duplicated per > > parameter, each test scenario can be run as an individual test so that you > > do not need extra plumbing to run each test scenario in the unlikely event > > of a failure. > > Okay, but there are a few other things to mention: > > One issue is the reset method is called at the end of each test. I think it > needs to be at the beginning of the method, moved to a finally block of a > try-finally, or maybe @BeforeMethod to reset before each test. The reason is > that one test failing will cause the tests that follow to fail too. Good catch, added `@BeforeMethod` > > The fields aren't constants so looks a bit strange (to me anyway) to be in > uppercase. If you rename them then I think the tests would be a bit more > readable. Ah, sometimes you see what you want. I must have had fields on my mind when I did that as I completely agree with you. Both of the above are addressed in the latest update to the PR. Thank you for the feedback. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Mon, 4 Jul 2022 19:30:40 GMT, Lance Andersen wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Address whitespace issue The updated TestDelegation test is looking a bit better now but I think it would be simplified a lot more by getting rid of the data providers, just aren't needed in this test. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Address whitespace issue - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/a51010aa..240c38b8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=05 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v5]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Separated test cases out and added benchmark - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/63b97ce3..a51010aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=04 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=03-04 Stats: 212 lines in 2 files changed: 198 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v4]
On Mon, 27 Jun 2022 16:43:55 GMT, Lance Andersen wrote:
>> Hi,
>>
>> Please review the following patch which will:
>>
>> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
>> the methods
>>
>> - public boolean exists(Path path, LinkOption... options)
>> - public A readAttributesIfExists(Path
>> path, Class type, LinkOption... options)
>>
>>
>> This change allows for providers to provide optimizations when the file's
>> attributes are not needed.
>>
>> Mach5 tiers 1 - 3 run clean with this change
>>
>> The CSR may be viewed at
>> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>>
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated tests based on feedback
test/jdk/java/nio/file/spi/TestDelegation.java line 111:
> 109: assertEquals(1, PROVIDER.findCall("exists").size());
> 110: assertEquals(2,
> PROVIDER.findCall("readAttributesIfExists").size());
> 111: PROVIDER.resetCalls();
One suggestion is to split these into testExists, testIsDirectory and
testIsRegularFile so that the delegation of each method is tested in isolation.
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v4]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updated tests based on feedback - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/6ef8bc1f..63b97ce3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=03 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=02-03 Stats: 293 lines in 3 files changed: 156 ins; 133 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v2]
On Sun, 26 Jun 2022 13:33:40 GMT, Lance Andersen wrote: > I thought about using TestProvider and then talked myself out of it. So yes, > I can do that. If you have a preferred name other than > `TestOverRideFSPMethods` please let me know The only change that should be needed to TestProvider is to implement the checkAccess method with: Path delegate = theFileSystem.unwrap(file); defaultProvider.checkAccess(delegate, modes); With that change it should be easy to extend and override exists/readAttributesIfExists to record that they are called. The need to run with the test with -Djava.nio.file.spi.DefaultFileSystemProvider=TestProvider goes away too and and should be much simpler to test that the expected delegation. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v3]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Switched Test to use TestProvideer - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/37b7462f..6ef8bc1f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=02 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=01-02 Stats: 596 lines in 3 files changed: 57 ins; 529 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v2]
On Sun, 26 Jun 2022 13:13:35 GMT, Alan Bateman wrote: > The implementation changes in latest version address my previous points, > thanks! I don't the test as it duplicates most of the existing of the > TestProvider in this directory and the naming is inconsistent with the > existing tests. I expect you should be able to just extent TestProvider and > override the two methods to record that they can been called. I thought about using TestProvider and then talked myself out of it. So yes, I can do that. If you have a preferred name other than `TestOverRideFSPMethods` please let me know - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v2]
On Sun, 26 Jun 2022 11:13:58 GMT, Lance Andersen wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on review feedback The implementation changes in latest version address my previous points, thanks! I don't the test as it duplicates most of the existing of the TestProvider in this directory and the naming is inconsistent with the existing tests. I expect you should be able to just extent TestProvider and override the two methods to record that they can been called. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v2]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updates based on review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/d2bd45c4..37b7462f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=01 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=00-01 Stats: 612 lines in 8 files changed: 603 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Thu, 23 Jun 2022 07:18:07 GMT, Alan Bateman wrote:
>> src/java.base/unix/classes/sun/nio/fs/UnixFileAttributes.java line 80:
>>
>>> 78: static UnixFileAttributes getIfExists(UnixPath path) throws
>>> UnixException {
>>> 79: UnixFileAttributes attrs = new UnixFileAttributes();
>>> 80: int errno = UnixNativeDispatcher.stat2(path, attrs);
>>
>> I think that this is the only use of `stat2()`. It could be deleted if this
>> method were replaced with this:
>>
>>
>> static UnixFileAttributes getIfExists(UnixPath path) throws
>> UnixException {
>> UnixFileAttributes attrs = new UnixFileAttributes();
>> try {
>> UnixNativeDispatcher.stat(path, attrs);
>> return attrs;
>> } catch (UnixException e) {
>> if (e.errno() == UnixConstants.ENOENT) {
>> return null;
>> }
>> throw e;
>> }
>> }
>
>> I think that this is the only use of `stat2()`. It could be deleted if this
>> method were replaced with this:
>
> The purpose of these additions to the SPI is to improve the performance of
> user facing methods that don't throw an exception when the file doesn't
> exist. It needs a stat/equivalent that doesn't throw so this is the reason
> for stat2. The equivalent on Windows will need to added too.
Understood.
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Thu, 23 Jun 2022 10:55:00 GMT, Alan Bateman wrote:
>> Hi,
>>
>> Please review the following patch which will:
>>
>> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
>> the methods
>>
>> - public boolean exists(Path path, LinkOption... options)
>> - public A readAttributesIfExists(Path
>> path, Class type, LinkOption... options)
>>
>>
>> This change allows for providers to provide optimizations when the file's
>> attributes are not needed.
>>
>> Mach5 tiers 1 - 3 run clean with this change
>>
>> The CSR may be viewed at
>> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>>
>>
>> Best,
>> Lance
>
> src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java line 306:
>
>> 304: case WRITE -> w = true;
>> 305: case EXECUTE -> x = true;
>> 306: default -> throw new AssertionError("Should not get
>> here");
>
> The switch should be exhaustive so I assume the default case is not required
> here.
Addressed in next push
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java line 302:
>
>> 300: throws IOException
>> 301: {
>> 302: return exists(path) ? readAttributes(path, type, options) :
>> null;
>
> If you add the following to ZipPath then it would allow you to implement it
> in one step rather than two:
>
>
> ZipFileAttributes readAttributesIfExits() throws IOException {
> return zfs.getFileAttributes(getResolvedPath());
> }
Added the proposed change above, Mach5 tiers 1-3 continue to be clean
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 22:26:23 GMT, Brian Burkhalter wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line > 1236: > >> 1234: * atomic operation with respect to other file system operations. >> 1235: * >> 1236: * @implSpec > > One more bad indentation. fixed in next push - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Thu, 23 Jun 2022 08:05:42 GMT, Chris Hegarty wrote:
>> Hi,
>>
>> Please review the following patch which will:
>>
>> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
>> the methods
>>
>> - public boolean exists(Path path, LinkOption... options)
>> - public A readAttributesIfExists(Path
>> path, Class type, LinkOption... options)
>>
>>
>> This change allows for providers to provide optimizations when the file's
>> attributes are not needed.
>>
>> Mach5 tiers 1 - 3 run clean with this change
>>
>> The CSR may be viewed at
>> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>>
>>
>> Best,
>> Lance
>
> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line
> 1202:
>
>> 1200: */
>> 1201: public boolean exists(Path path, LinkOption... options) {
>> 1202: try {
>
> Overall, I think that this is a great change (avoiding the need for various
> parts of the system to communicate through exceptions).
>
> Trivially, and easily, I often find it useful to add mock-like tests for
> these specified implementations (implSpec), i.e. just subclass FSP mocking
> out the abstract methods to ensure that the default implementations invoke
> the appropriate ones.
Thank you for the suggestion Chris
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java line 302:
> 300: throws IOException
> 301: {
> 302: return exists(path) ? readAttributes(path, type, options) : null;
If you add the following to ZipPath then it would allow you to implement it in
one step rather than two:
ZipFileAttributes readAttributesIfExits() throws IOException {
return zfs.getFileAttributes(getResolvedPath());
}
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
This should be performance neutral for the default provider and positive for
the zip provider. Do we have any micros that could be added?
The end date in the copyright header will need updated a few files, I assume
you'll do that before this change is integrated.
src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java line 306:
> 304: case WRITE -> w = true;
> 305: case EXECUTE -> x = true;
> 306: default -> throw new AssertionError("Should not get
> here");
The switch should be exhaustive so I assume the default case is not required
here.
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line 1202:
> 1200: */
> 1201: public boolean exists(Path path, LinkOption... options) {
> 1202: try {
Overall, I think that this is a great change (avoiding the need for various
parts of the system to communicate through exceptions).
Trivially, and easily, I often find it useful to add mock-like tests for these
specified implementations (implSpec), i.e. just subclass FSP mocking out the
abstract methods to ensure that the default implementations invoke the
appropriate ones.
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 22:18:05 GMT, Brian Burkhalter wrote: > I think that this is the only use of `stat2()`. It could be deleted if this > method were replaced with this: The purpose of these additions to the SPI is to improve the performance of user facing methods that don't throw an exception when the file doesn't exist. It needs a stat/equivalent that doesn't throw so this is the reason for stat2. The equivalent on Windows will need to added too. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote: > Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line 1236: > 1234: * atomic operation with respect to other file system operations. > 1235: * > 1236: * @implSpec One more bad indentation. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
src/java.base/unix/classes/sun/nio/fs/UnixFileAttributes.java line 80:
> 78: static UnixFileAttributes getIfExists(UnixPath path) throws
> UnixException {
> 79: UnixFileAttributes attrs = new UnixFileAttributes();
> 80: int errno = UnixNativeDispatcher.stat2(path, attrs);
I think that this is the only use of `stat2()`. It could be deleted if this
method were replaced with this:
static UnixFileAttributes getIfExists(UnixPath path) throws UnixException {
UnixFileAttributes attrs = new UnixFileAttributes();
try {
UnixNativeDispatcher.stat(path, attrs);
return attrs;
} catch (UnixException e) {
if (e.errno() == UnixConstants.ENOENT) {
return null;
}
throw e;
}
}
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:54:47 GMT, Lance Andersen wrote: >> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line >> 1218: >> >>> 1216: >>> 1217: /** >>> 1218: * Reads a file's attributes as a bulk operation if it exists. >> >> Maybe instead "it" put "the file"? > > I can if you think that is better. I followed the wording for > `FileSystemProvider::deleteIfExists` trying to be consistent Then leave is as is. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:22:50 GMT, Brian Burkhalter wrote:
>> Hi,
>>
>> Please review the following patch which will:
>>
>> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
>> the methods
>>
>> - public boolean exists(Path path, LinkOption... options)
>> - public A readAttributesIfExists(Path
>> path, Class type, LinkOption... options)
>>
>>
>> This change allows for providers to provide optimizations when the file's
>> attributes are not needed.
>>
>> Mach5 tiers 1 - 3 run clean with this change
>>
>> The CSR may be viewed at
>> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>>
>>
>> Best,
>> Lance
>
> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line
> 1184:
>
>> 1182: * is present then symbolic links are not followed and the method
>> 1183: * {@link #readAttributes(Path, Class, LinkOption...)} is called
>> 1184: * to determine whether a file exists.
>
> Indentation is off at lines 1183-1184.
Thanks for catching that. Will be addressed in the next push
> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line
> 1218:
>
>> 1216:
>> 1217: /**
>> 1218: * Reads a file's attributes as a bulk operation if it exists.
>
> Maybe instead "it" put "the file"?
I can if you think that is better. I followed the wording for
`FileSystemProvider::deleteIfExists` trying to be consistent
-
PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote:
> Hi,
>
> Please review the following patch which will:
>
> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include
> the methods
>
> - public boolean exists(Path path, LinkOption... options)
> - public A readAttributesIfExists(Path
> path, Class type, LinkOption... options)
>
>
> This change allows for providers to provide optimizations when the file's
> attributes are not needed.
>
> Mach5 tiers 1 - 3 run clean with this change
>
> The CSR may be viewed at
> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336)
>
>
> Best,
> Lance
src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line 1184:
> 1182: * is present then symbolic links are not followed and the method
> 1183: * {@link #readAttributes(Path, Class, LinkOption...)} is called
> 1184: * to determine whether a file exists.
Indentation is off at lines 1183-1184.
src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java line 1218:
> 1216:
> 1217: /**
> 1218: * Reads a file's attributes as a bulk operation if it exists.
Maybe instead "it" put "the file"?
-
PR: https://git.openjdk.org/jdk/pull/9249
