feat: Change checksum algorithm to CRC32C. And localize checksum handling t… #506
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
product-auto-label
bot
added
the
api: datastore
google-cla
bot
added
the
cla: yes
vnorigoog
changed the title
kolea2
suggested changes
Aug 24, 2021
| return hc.toString(); | ||
| } | ||
|
|
||
| private static Hasher getNewHasher() { |
There was a problem hiding this comment.
for better readability, I would rename this to getNewCrc32cHasher
...re-v1-proto-client/src/main/java/com/google/datastore/v1/client/EndToEndChecksumHandler.java
Show resolved
Hide resolved
kolea2
changed the title
vnorigoog
commented
Aug 31, 2021
kolea2
reviewed
Sep 2, 2021
datastore-v1-proto-client/src/test/java/com/google/datastore/v1/client/RemoteRpcTest.java
Show resolved
Hide resolved
crwilcox
reviewed
Sep 2, 2021
datastore-v1-proto-client/src/test/java/com/google/datastore/v1/client/RemoteRpcTest.java
Show resolved
Hide resolved
...to-client/src/test/java/com/google/datastore/v1/client/ChecksumEnforcingInputStreamTest.java
Show resolved
Hide resolved
|
appreciate follow-up feedback on the PR please
…On Thu, Sep 2, 2021 at 12:38 PM Christopher Wilcox ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
datastore-v1-proto-client/src/test/java/com/google/datastore/v1/client/RemoteRpcTest.java
<#506 (comment)>
:
> @@ -174,7 +174,7 @@ public void testHttpHeaders_expectE2eChecksumHeader() throws IOException {
httpRequest
.getHeaders()
.getFirstHeaderStringValue(EndToEndChecksumHandler.HTTP_REQUEST_CHECKSUM_HEADER);
- assertEquals(32, header.length());
+ assertEquals(8, header.length());
we have also recently been moving to crc32c for other libs, as the
computation should be faster for streams.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#506 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALRAMQU4UZQAWFJN4ZWIBODT77HCBANCNFSM5CRAS2SA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
kolea2
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
crwilcox
approved these changes
Sep 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…o one file.