NIO Path support #304
NIO Path support #304
Conversation
There was a problem hiding this comment.
This looks good. When you point it out, it is odd that I converted to FIle this cleans that up nicely and allows support for other file systems so that's nice. Do you have a use case for this change?
It seems there are a few failing tests hopefully mostly where a File is now compared to a Path could you take a look and see if you can fix those?
About the API i'm not too attached to it ATM I know of quite a few users, but if you have a strong proposal to change it, and keep it simple I would be happy to do so. Then again the solution proposed here doesn't break it so maybe that's nicer.
Think there are also some minor CI checkstyle issues would be great if you can take a look.
| // many file systems do not support memory mapping | ||
| int lengthAsInt = (int) length; | ||
| if (lengthAsInt != length) { | ||
| // length must be representable by an int such that we can allocate a ByteBuffer manually | ||
| throw e; | ||
| } | ||
| ByteBuffer buffer = ByteBuffer.allocate(lengthAsInt); | ||
| long oldPosition = fc.position(); | ||
| fc.position(address); | ||
| try { | ||
| int totalNumberOfBytesRead = 0; | ||
| while (totalNumberOfBytesRead < lengthAsInt) { | ||
| int numberOfBytesRead = fc.read(buffer); | ||
| if (numberOfBytesRead < 0) { | ||
| throw new IOException("Trying to read more bytes than available"); | ||
| } | ||
| totalNumberOfBytesRead += numberOfBytesRead; | ||
| } | ||
| } | ||
| finally { | ||
| // reset file channel to old position | ||
| fc.position(oldPosition); | ||
| } | ||
| buffer.flip(); | ||
| return buffer; |
There was a problem hiding this comment.
Think maybe this is a cleaner impl
ByteBuffer buffer = ByteBuffer.allocate(Math.toIntExact(length));
int bytesRead = fc.read(buffer, address);
if(bytesRead != length) {
throw new IOException("Incorrect bytes read");
}
buffer.flip();
return buffer;
What do you think?
There was a problem hiding this comment.
Regarding Math.toIntExact(length): If this conversion fails, then the caller will get an ArithmeticException. This might be a bit surprising, so I tried to handle this case individually. However, since HdfInMemoryByteBuffer.mapNoOffset() also uses this method, I think that it is consistent to use it here as well.
Regarding your suggestion to remove the loop for reading data into the ByteBuffer: FileChannel.read() does not guarantee to fill the whole buffer. It might even return 0 although there is still more data to read. Maybe this case occurs when data is transferred via the network and it is known that there is more data, but it has not arrived yet. This is why I think it is safer to read the data in a loop.
Regarding resetting the position: I tried to avoid side effects, so I decided to reset the position after reading the data. If you think that this side effect is uncritical, then we could remove this behavior to make the code simpler. What do you think?
| * </li> | ||
| * </ol> | ||
| */ | ||
| class NioPathTest |
There was a problem hiding this comment.
Test looks great really thorough. Would it be possible to convert to Hamcrest style assetThat assertions. I think there more readable and have generally nicer errors. Not a blocker though can be merged anyway and changed later.
There was a problem hiding this comment.
I changed the test to Hamcrest style to be consistent with the other tests. Unlike the other tests, I used Matchers.is() as a shortcut for Matchers.is(Matchers.equalTo()). If you find is(equalTo()) more expressive in the code, then I can also use the longer version.
|
Our company has developed a distributed file system tailored to measurement data. We have custom FileSystem and Path implementations to access the files there. Additionally, many cloud provides have an NIO-based Java API. I am sorry for the failing tests. I do not know why I forgot to run all tests before creating the pull request... |
…ileChannel.mapNoOffset() into separate method
…memory mapping has failed earlier
|
Think this just need this adding 9fc4021 then the tests will pass and think this can be merged. |
|
SonarCloud Quality Gate failed. |
jHDF makes intensive use of NIO internally and HdfFile provides constructors for Path and URI. However, these constructors convert Path and URI to File (before converting it back to Path again). In this branch, all internal usages of File have been replaced with Path.
Remarks: