Skip to content

Conversation

cody-littley
Copy link
Contributor

Why are these changes needed?

Switch to siphash for littDB sharding.

Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley self-assigned this Apr 28, 2025
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley requested a review from litt3 May 7, 2025 15:04
@cody-littley cody-littley marked this pull request as ready for review May 7, 2025 15:04
@cody-littley cody-littley requested a review from anupsv May 7, 2025 15:04
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this file ? or would it be generated during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All data beneath testdata is automatically generated. The purpose of all of these files is to simulate data migration from one format to another. I generated these files using the old serialization format in master, and use the code in this branch to read the data back again. The contents of this directory should be snapshots of the LittDB's file system taken at each version.

As to specifically whether we need this file, I'm not sure. It's generated internally by levelDB. I'm hoping to avoid having to hand prune these files.

// The function performs a recursive copy of all files and directories, maintaining the same
// relative path structure and file metadata. If the destination directory exists, it will
// merge the source content into it, potentially overwriting files with the same names.
func CopyDirectoryRecursively(src, dst string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

this assumes the dst is writable right ? Should we check if it is and error out ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this check. I also added some unit tests to verify.

}
hash, err := hasher.Write(key)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we wanna panic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Place holder from when I initially wrote this, but I forgot to circle back and fix it. This method now returns an error.

@cody-littley cody-littley requested a review from anupsv May 9, 2025 14:54
"encoding/binary"
"fmt"

"github.com/aead/siphash"
Copy link
Contributor

Choose a reason for hiding this comment

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

im wondering if we should use this lib of siphash https://github.com/dchest/siphash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.


hash, err := util.HashKey(key, s.metadata.salt)
if err != nil {
return 0, fmt.Errorf("failed to hash key: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

%v -> %w (also elsewhere in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced a bunch of %vs in segment.go (many of which were not introduced in this PR), but didn't find any others.

metadata.GetShardingFactor(),
config.SaltShaker.Uint32(),
([16]byte)(salt),
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

// The function checks that the destination has appropriate write permissions before starting the copy.
// If the destination directory doesn't exist, it verifies the parent directory has appropriate permissions.
// For existing directories, it ensures they have write permissions before attempting to copy files into them.
func CopyDirectoryRecursively(source string, destination string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be nice to have all exported functions at the top, so it's clear what is "important" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley merged commit fb43113 into master May 12, 2025
15 checks passed
@cody-littley cody-littley deleted the littdb-siphash branch May 12, 2025 19:39
ethenotethan pushed a commit that referenced this pull request May 14, 2025
* use siphash for sharding

Signed-off-by: Cody Littley <[email protected]>

* Start work on migration test.

Signed-off-by: Cody Littley <[email protected]>

* Generate table using new format

Signed-off-by: Cody Littley <[email protected]>

* Added migration test, need to debug

Signed-off-by: Cody Littley <[email protected]>

* fix migration

Signed-off-by: Cody Littley <[email protected]>

* made suggested changes

Signed-off-by: Cody Littley <[email protected]>

* Made suggested changes.

Signed-off-by: Cody Littley <[email protected]>

* Change siphash implementation.

Signed-off-by: Cody Littley <[email protected]>

* made suggested changes

Signed-off-by: Cody Littley <[email protected]>

* fix go.sum

Signed-off-by: Cody Littley <[email protected]>

---------

Signed-off-by: Cody Littley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants