Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RandomAccessFile digest methods #31

Closed
wants to merge 1 commit into from
Closed

Add RandomAccessFile digest methods #31

wants to merge 1 commit into from

Conversation

behrangsa
Copy link

@behrangsa behrangsa commented Nov 30, 2019

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.

@coveralls
Copy link

coveralls commented Nov 30, 2019

Coverage Status

Coverage increased (+0.01%) to 93.339% when pulling 8ee6510 on behrangsa:master into 3ab9ce4 on apache:master.

@behrangsa
Copy link
Author

Not sure why the coveralls is reporting that the new methods are not tested. Tests have been added for both of them.

Copy link
Contributor

@aherbert aherbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should call nonblockingDigest.

@aherbert
Copy link
Contributor

I am also not sure why coveralls dropped. Looking at the Travis log for JDK 8 it runs the tests:

[INFO] Running org.apache.commons.codec.digest.MessageDigestAlgorithmsTest
[WARNING] Tests run: 88, Failures: 0, Errors: 0, Skipped: 32, Time elapsed: 4.816 s - in org.apache.commons.codec.digest.MessageDigestAlgorithmsTest

When I run locally mvn clean test -Dtest=MessageDigestAlgorithmsTest jacoco:report I get the same numbers of tests run (those skipped are for algorithms in later JDKs) and the local jacoco report has coverage of the new methods.

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.

@behrangsa
Copy link
Author

Silly me. Thanks for the review.

@behrangsa
Copy link
Author

@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()
                )
        );
    }

@aherbert
Copy link
Contributor

The failure is due to the post travis action to build the coveralls report. This command:

mvn clean cobertura:cobertura coveralls:report -Ptravis-cobertura

Fails to run the test:

[INFO] Running org.apache.commons.codec.digest.MessageDigestAlgorithmsTest
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.001 s <<< FAILURE! - in org.apache.commons.codec.digest.MessageDigestAlgorithmsTest
[ERROR] org.apache.commons.codec.digest.MessageDigestAlgorithmsTest  Time elapsed: 0.001 s  <<< ERROR!
java.lang.ClassCastException: [I cannot be cast to java.lang.String
	at org.apache.commons.codec.digest.MessageDigestAlgorithmsTest.checkValues(MessageDigestAlgorithmsTest.java:71)

This is a problem on master that needs fixing. It is not related to your PR.

@aherbert
Copy link
Contributor

I fixed master to work with cobertura. If you rebase you should get coveralls reporting.

@garydgregory
Copy link
Member

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.

@garydgregory
Copy link
Member

Let's step back and ask: Why would we want two implementations for File objects? If NIO does the job better, then let's change the implementation to do that.

@behrangsa
Copy link
Author

Let's step back and ask: Why would we want two implementations for File objects? If NIO does the job better, then let's change the implementation to do that.

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:

  • Get rid of nonblockingDigest(final MessageDigest messageDigest, final File data)
  • Rename nonblockingDigest(final MessageDigest messageDigest, final RandomAccessFile data) to digest(final MessageDigest messageDigest, final RandomAcessFile data)

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.
@behrangsa behrangsa changed the title Added non-blocking File digest methods Added RandomAccessFile digest methods Dec 4, 2019
@garydgregory garydgregory changed the title Added RandomAccessFile digest methods Add RandomAccessFile digest methods Dec 4, 2019
@garydgregory
Copy link
Member

@asfgit asfgit closed this in 625cedf Dec 4, 2019
asfgit pushed a commit that referenced this pull request Dec 4, 2019
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants