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

Conversation

@behrangsa
Copy link

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Author

behrangsa commented Nov 30, 2019

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 left a 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()));

This comment has been minimized.

Copy link
@aherbert

aherbert Nov 30, 2019

Contributor

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

This comment has been minimized.

Copy link
@aherbert

aherbert Nov 30, 2019

Contributor

This should call nonblockingDigest.

@aherbert

This comment has been minimized.

Copy link
Contributor

aherbert commented Nov 30, 2019

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

This comment has been minimized.

Copy link
Author

behrangsa commented Nov 30, 2019

Silly me. Thanks for the review.

@behrangsa

This comment has been minimized.

Copy link
Author

behrangsa commented Nov 30, 2019

@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

This comment has been minimized.

Copy link
Contributor

aherbert commented Nov 30, 2019

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

This comment has been minimized.

Copy link
Contributor

aherbert commented Nov 30, 2019

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

@behrangsa behrangsa force-pushed the behrangsa:master branch 2 times, most recently from 0f1e1fa to 2a2aed0 Nov 30, 2019
@garydgregory

This comment has been minimized.

Copy link
Member

garydgregory commented Nov 30, 2019

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

This comment has been minimized.

Copy link
Member

garydgregory commented Dec 1, 2019

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

This comment has been minimized.

Copy link
Author

behrangsa commented Dec 3, 2019

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.

@behrangsa behrangsa force-pushed the behrangsa:master branch 2 times, most recently from 824490b to 5b19b1b Dec 3, 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.
@behrangsa behrangsa force-pushed the behrangsa:master branch from 5b19b1b to 8ee6510 Dec 4, 2019
@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

This comment has been minimized.

Copy link
Member

garydgregory commented Dec 4, 2019

@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
Projects
None yet
4 participants
You can’t perform that action at this time.