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

ISSUE #1075: Add a noop digest implentation #1087

Closed
wants to merge 1 commit into from

Conversation

athanatos
Copy link

This digest will add a predefined digest key without actually computing
it. This can be used for testing or in situations/use-cases where digest
is considered overhead.

(@bug W-3245776@)
Signed-off-by: Venkateswararao Jujjuri (JV) vjujjuri@salesforce.com
[Adapted to current patch]
Signed-off-by: Samuel Just sjust@salesforce.com


@Override
int getMacCodeLength() {
return 8;
Copy link
Member

Choose a reason for hiding this comment

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

any reason why a dummy digest manager needs a 8 bytes here? can't we just make it 0? the digest here is anyway useless, no?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure. Perhaps there is value in emulating the size overhead? @jvrao Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't really recall why we made up the digest and kept it at 8. Maybe tried to keep the payload size in sync with CRC32.

Copy link
Member

Choose a reason for hiding this comment

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

@jvrao any idea to waste extra 4 bytes, if it is not even used.

Copy link
Author

Choose a reason for hiding this comment

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

@jvrao I'll switch it to an empty payload.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with it.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Maybe it is better to add a test on new API as well. Just to cover the new constant usage in tests

@athanatos
Copy link
Author

@eolivelli Repushed with new api test.

@sijie
Copy link
Member

sijie commented Feb 5, 2018

@eolivelli please review this since you were involved in this PR

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Good the new test case and the 0 length payload.
I left one last comment please check it

@Test
public void testLedgerDigests() throws Exception {
DigestType[] types = new DigestType[] {
DigestType.CRC32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sine this is an enum we should use DigestType.values() so that we are sure to cover new types in the future

This digest will add a predefined digest key without actually computing
it. This can be used for testing or in situations/use-cases where digest
is considered overhead.

(@bug W-3245776@)
Signed-off-by: Venkateswararao Jujjuri (JV) <vjujjuri@salesforce.com>
[Adapted to current patch, added tests]
Signed-off-by: Samuel Just <sjust@salesforce.com>
@athanatos
Copy link
Author

@eolivelli Done.

@sijie
Copy link
Member

sijie commented Feb 10, 2018

ping @eolivelli

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1 looks good thank you

@eolivelli
Copy link
Contributor

retest this please

1 similar comment
@sijie
Copy link
Member

sijie commented Feb 14, 2018

retest this please

@asfgit
Copy link

asfgit commented Feb 16, 2018

Can one of the admins verify this patch?

@sijie
Copy link
Member

sijie commented Feb 16, 2018

retrigger travis ci build

@sijie
Copy link
Member

sijie commented Feb 16, 2018

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants