-
Notifications
You must be signed in to change notification settings - Fork 55
Use HashingSource and HashingSink #788
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
Conversation
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
The failing e2e test is unrelated to this change, and I'm working on fixing it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, let's get e2e tests and service check batch tests working before merging though. (In particular e2e tests as it covers two different event stream services/operations)
| private fun emptyByteArray(): ByteArray = ByteArray(0) | ||
|
|
||
| // Convert a big-endian ByteArray to an Int | ||
| private fun ByteArray.toInt(): Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: you could make this internal and share it with your unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the .toInt() conversion entirely, now just writing the raw bytes out using write() instead of writeInt()
| "type": "feature", | ||
| "description": "Add usages of HashingSource and HashingSink" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: This isn't a great changelog description of the change. Is there a meaningful difference here like performance improvements? If not, this may be something that doesn't deserve a changelog entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, since it's a small change and not really customer-facing (no performance improvements / other effects expected), I will exclude the changelog
| dest.writeInt(sink.crc.toInt()) | ||
| dest.writeInt(sink.digest().toInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why writeInt with a toInt conversion? Why not write which takes a byte array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tip, this will work and make it simpler. I was converting the big endian ByteArray to an integer when I could just write the bytes out directly
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
A new generated diff is ready to view.
|








Change usage of CrcSource/CrcSink to HashingSource and HashingSink.
Issue #
Description of changes
This change is required to use the HashingSource and HashingSink implemented in the
smithy-kotlinpair PR: smithy-lang/smithy-kotlin#756By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.