-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix(random): replacing with depricated math/rand.Read until we need to change it #169
Conversation
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.
Pull Request Overview
This PR temporarily reverts to using the legacy math/rand.Read for random data generation while investigating proper initialization for math/rand/v2.ChaCha8.
- Updated import alias to reference math/rand as a fallback.
- Replaced the ChaCha8.Read call with math/rand.Read including an inline nolint directive.
6ff08e0
to
7a50c55
Compare
7a50c55
to
a549ed9
Compare
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.
Pull Request Overview
This PR provides a temporary fallback for generating random data while upgrading to Go 1.24 and switching to math/rand/v2 by reverting to the old math/rand.Read method until a proper initialization for ChaCha8 is implemented. Key changes include:
- Updating random number generation sources in random.go to use math/rand/v2 and introducing LockedRandom and LockedRandomString types.
- Modifying GenerateData and related functions in modes.go to return errors instead of panicking.
- Adjusting tests, benchmarks, main.go, and the golangci configuration for the new Go version.
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
random/random_test.go | Added extra imports and tests for LockedRandomString generation. |
random/random.go | Refactored randomness generators with new types and initialization, switching to math/rand/v2. |
modes.go | Updated GenerateData and data validation to return errors and included safety checks. |
main.go | Exited with a status code from the final test result. |
.golangci.yml | Updated Go version to 1.24. |
Files not reviewed (3)
- Dockerfile: Language not supported
- Makefile: Language not supported
- go.mod: Language not supported
…o change it Signed-off-by: Dusan Malusev <[email protected]>
Signed-off-by: Dusan Malusev <[email protected]>
df99151
to
016b539
Compare
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.
Pull Request Overview
This PR updates the random number generation by falling back to the deprecated math/rand.Read until a proper initialization for math/rand/v2.ChaCha8 is implemented. Key changes include updating random generator implementations in the random package, adding error handling in data generation, and adjusting tests and benchmark functions.
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
random/random_test.go | Updates tests and benchmarks to use the new random generators |
random/random.go | Switches from math/rand to math/rand/v2, implements new locked random types |
modes.go | Modifies GenerateData to return errors and updates query binding in workloads |
main.go | Changes final status handling to an os.Exit call |
.golangci.yml | Updates the Go version to 1.24 |
Files not reviewed (3)
- Dockerfile: Language not supported
- Makefile: Language not supported
- go.mod: Language not supported
While upgrading to go 1.24 and to the new
math/rand/v2
package,math/rand.Read
has been replaced withmath/rand/v2.ChaCha8.Read
but it's not properly initialized to generate good random data. Until this is figured out, fallback tomath/rand.Read
is requiredFixes #168
Argus Test: https://argus.scylladb.com/tests/scylla-cluster-tests/5227cd40-b27d-4c19-bc23-3ad580a1a18d (now scylla-bench does not fail, test fails for different reasons