Add RandomAccessFile digest methods#31
Conversation
|
Not sure why the coveralls is reporting that the new methods are not tested. Tests have been added for both of them. |
aherbert
left a comment
There was a problem hiding this comment.
Your tests do not call the new methods.
| Assume.assumeTrue(DigestUtils.isAvailable(messageDigestAlgorithm)); | ||
| Assert.assertArrayEquals(digestTestData(), | ||
| DigestUtils.nonblockingDigest(DigestUtils.getDigest(messageDigestAlgorithm), getTestRandomAccessFile())); | ||
| Assert.assertArrayEquals(digestTestData(), DigestUtils.digest(DigestUtils.getDigest(messageDigestAlgorithm),getTestFile())); |
There was a problem hiding this comment.
This should call nonblockingDigest with getTestRandomAccessFile()
| Assume.assumeTrue(DigestUtils.isAvailable(messageDigestAlgorithm)); | ||
| Assert.assertArrayEquals(digestTestData(), | ||
| DigestUtils.nonblockingDigest(DigestUtils.getDigest(messageDigestAlgorithm), getTestFile())); | ||
| Assert.assertArrayEquals(digestTestData(), DigestUtils.digest(DigestUtils.getDigest(messageDigestAlgorithm),getTestFile())); |
There was a problem hiding this comment.
This should call nonblockingDigest.
|
I am also not sure why coveralls dropped. Looking at the Travis log for JDK 8 it runs the tests: When I run locally If you fix the new tests to make a duplicate call to the new NIO methods and push it will run again and we can see if this is some strange glitch in coveralls. |
|
Silly me. Thanks for the review. |
|
@aherbert I fixed those issues, but for some reason coveralls reports that they are not covered yet: @Test
public void testNonBlockingDigestRandomAccessFile() throws IOException {
Assume.assumeTrue(DigestUtils.isAvailable(messageDigestAlgorithm));
final byte[] expected = digestTestData();
Assert.assertArrayEquals(expected,
DigestUtils.nonblockingDigest(
DigestUtils.getDigest(messageDigestAlgorithm), getTestRandomAccessFile()
)
);
Assert.assertArrayEquals(expected,
DigestUtils.nonblockingDigest(
DigestUtils.getDigest(messageDigestAlgorithm), getTestRandomAccessFile()
)
);
}
@Test
public void testNonBlockingDigestFile() throws IOException {
Assume.assumeTrue(DigestUtils.isAvailable(messageDigestAlgorithm));
final byte[] expected = digestTestData();
Assert.assertArrayEquals(
expected,
DigestUtils.nonblockingDigest(
DigestUtils.getDigest(messageDigestAlgorithm), getTestFile()
)
);
Assert.assertArrayEquals(
expected,
DigestUtils.nonblockingDigest(
DigestUtils.getDigest(messageDigestAlgorithm), getTestFile()
)
);
} |
|
The failure is due to the post travis action to build the coveralls report. This command: Fails to run the test: This is a problem on master that needs fixing. It is not related to your PR. |
|
I fixed master to work with cobertura. If you rebase you should get coveralls reporting. |
|
I am fine with adding more methods to handing more inputs but I am -1 to putting implementation details like "NIO" in and "nonblocking" in method names. |
|
Let's step back and ask: Why would we want two implementations for |
I thought using NIO would improve performance but in my limited tests I didn't notice a meaningful difference. It would be great if some NIO experts could chime in. However we can:
as the first method calls wraps the File in a RandomAccessFile and passes it to the 2nd method anyway. |
Added two new methods: * digest(final MessageDigest messageDigest, final RandomAccessFile data) * updateDigest(final MessageDigest digest, final RandomAccessFile data) For computing digest of RandomAccessFiles. Performance-wise there's almost no improvements offered by these new NIO based methods, but being non-blocking they could potentially use fewer resources and provide better throughput in highly multi-threaded scenarios.
- This is a slightly different version from #31 - Refactor updateDigest(MessageDigest,RandomAccessFile) into an new private updateDigest(MessageDigest,FileChannel) as possible public candidate. - Do NOT seek to 0 on a RandomAccessFile before calling updateDigest(): We do not do this for ByteBuffer input, so do not do it here and be consistent to assume that when the caller says 'digest this' then do it from where the input stands (like a stream). - Add methods in the file to keep methods in alphabetical order. - Closes #31.
- This is a slightly different version from apache#31 - Refactor updateDigest(MessageDigest,RandomAccessFile) into an new private updateDigest(MessageDigest,FileChannel) as possible public candidate. - Do NOT seek to 0 on a RandomAccessFile before calling updateDigest(): We do not do this for ByteBuffer input, so do not do it here and be consistent to assume that when the caller says 'digest this' then do it from where the input stands (like a stream). - Add methods in the file to keep methods in alphabetical order. - Closes apache#31.
- This is a slightly different version from apache#31 - Refactor updateDigest(MessageDigest,RandomAccessFile) into an new private updateDigest(MessageDigest,FileChannel) as possible public candidate. - Do NOT seek to 0 on a RandomAccessFile before calling updateDigest(): We do not do this for ByteBuffer input, so do not do it here and be consistent to assume that when the caller says 'digest this' then do it from where the input stands (like a stream). - Add methods in the file to keep methods in alphabetical order. - Closes apache#31.
Added two new methods:
digest(final MessageDigest messageDigest, final RandomAccessFile data)updateDigest(final MessageDigest digest, final RandomAccessFile data)For computing digest of
RandomAccessFiles.Performance-wise there's almost no improvements offered by these new NIO based methods, but being non-blocking they could potentially use fewer resources and provide better throughput in highly multi-threaded scenarios.