Switch to using blake3 hashes and skipping empty blocks #29
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
awels
force-pushed
the
switch_blake3
branch
from
315b90d to
57482b7
Compare
|
/cc @rayfordj If you could review? |
dymurray
reviewed
Oct 8, 2024
| f.log.V(5).Info("Comparing hash of hashes", "hash", base64.StdEncoding.EncodeToString(f.hashHash)) | ||
| if n, err := rw.Write(f.hashHash); err != nil { | ||
| return false, err | ||
| } else { |
There was a problem hiding this comment.
NIT: no need for an else statement here
There was a problem hiding this comment.
Actually I do need the else here, because I am referencingn which only exists in the scope of the if statement.
dymurray
reviewed
Oct 8, 2024
| if n, err := io.ReadFull(rw, hashHash); err != nil { | ||
| return false, err | ||
| } else { | ||
| f.log.V(5).Info("Read hash of hashes", "bytes", n, "hash", base64.StdEncoding.EncodeToString(hashHash)) |
There was a problem hiding this comment.
Same reason the else exists.
dymurray
reviewed
Oct 8, 2024
pkg/blockrsync/hasher_test.go
Outdated
| @@ -64,7 +64,7 @@ var _ = Describe("hasher tests", func() { | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| hashes := hasher.GetHashes() | |||
| // 16 for the blocksize and length, 72 for each hash | |||
dymurray
approved these changes
Oct 8, 2024
Blake3 hashes are significantly faster than blake2 hashes used previously. Also added faster empty block detection so those don't need to get hashed at all. Signed-off-by: Alexander Wels <[email protected]>
|
Fixed the comment, the elses are still needed because of a variable only visible in the scope of the if statement. |
pranavgaikwad
approved these changes
Oct 8, 2024
pkg/blockrsync/hasher.go
Outdated
| @@ -30,6 +36,7 @@ type Hasher interface { | |||
| SerializeHashes(io.Writer) error | |||
| DeserializeHashes(io.Reader) (int64, map[int64][]byte, error) | |||
| BlockSize() int64 | |||
| CompareHashHash(io.ReadWriter) (bool, error) | |||
There was a problem hiding this comment.
find this name confusing, maybe s/HashHash/HashOfHash/
Renamed hashhash to hashofhash Signed-off-by: Alexander Wels <[email protected]>
dymurray
approved these changes
Oct 16, 2024
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.
Blake3 hashes are significantly faster than blake2 hashes used previously. Also added faster empty block detection so those don't need to get hashed at all.