-
Notifications
You must be signed in to change notification settings - Fork 442
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
Upgrade Go version to v1.20 #2190
Conversation
Signed-off-by: Yuki Iwai <[email protected]>
func NewWithSQLConn(db *sql.DB) (common.KatibDBInterface, error) { | ||
d := new(dbConn) | ||
d.db = db | ||
seed, err := crand.Int(crand.Reader, big.NewInt(1<<63-1)) | ||
if err != nil { | ||
return nil, fmt.Errorf("RNG initialization failed: %v", err) | ||
} | ||
// We can do the following instead, but it creates a locking issue | ||
//d.rng = rand.New(rand.NewSource(seed.Int64())) | ||
rand.Seed(seed.Int64()) | ||
|
||
return d, nil | ||
} |
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.
We need to replace the rand.Seed()
function with another way since the rand.Seed()
function is deprecated, as shown below.
Deprecated: Programs that call Seed and then expect a specific sequence of results from the global random source (using functions such as Int) can be broken when a dependency changes how much it consumes from the global random source. To avoid such breakages, programs that need a specific result sequence should use NewRand(NewSource(seed)) to obtain a random generator that other packages cannot access.
https://pkg.go.dev/math/rand#Seed
However, I don't think we need to set the global random seed to the same value every run since we don't use random values anywhere, as I can see. So, I removed this.
@andreyvelich @johnugeorge WDYT?
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 am not sure why did we set these values.
@gaocegege @johnugeorge Do you have any context ?
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.
@andreyvelich As I can see, I can not find any context for this random number by git blame.
It seems that this was copied from https://github.com/mlkube/katib/blob/0bed55aae3cfddf255e6d297e31d1fedbe5dca4a/db/interface.go#L65-L67 to here.
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.
/lgtm
/assign @johnugeorge @gaocegege
/hold for review
/lgtm Thanks for your contribution! 🎉 👍 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, gaocegege, tenzen-y The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
What this PR does / why we need it:
I upgraded Go version to v1.20.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Checklist: