Fix getJitter panic: invalid argument to Int63n #389
Conversation
|
Code Climate has analyzed commit 58a0d65 and detected 0 issues on this pull request. View more on Code Climate. |
5a16a30 to
6ca5b37
Compare
periodic/periodic.go
Outdated
| if t <= 0 { | ||
| return time.Duration(0) | ||
| } | ||
| i := int64(t / 10) |
There was a problem hiding this comment.
this probably should be something like
func getJitter(t time.Duration) time.Duration {)
i:= int64(float64(t)/10.+ 0.5) // round to nearest instead of truncate
if i <= 0 {
return time.Duration(0)
}
j := rand.Int63n(2*i) - i
return time.Duration(j)
}ie not test for 0 twice
(could also use math.Ceil and maybe use /5 and 2*i - i/2 instead of i/2 and -i) or use rand.Float64()...)
There was a problem hiding this comment.
or could check for <= 5 (or 10) at the beginning
There was a problem hiding this comment.
maybe something like:
func getJitter(t time.Duration) time.Duration {
if t < 10 {
return time.Duration(0)
}
i:= int64(t/10) # if t < 10 check to make sure i > 0
j := rand.Int63n(2*i) - i
return time.Duration(j)
}
do not add +0.5, since when float64(t)/10+ 0.5 is a constant, golang will throw constant xx.xx truncated to integercompile error, as the constant does not fit into the specified type (int64).
There was a problem hiding this comment.
odd, /10. should keep everything float until after the int64() but not a huge deal
I wonder if you can you add a test that recreates the panic before the fix and shows the fix but it may be tricky to get sleep to be < 10 yet > 0
There was a problem hiding this comment.
weird, i cannot reproduce the previous error i got. Yes, that looks fine. Will update!
There was a problem hiding this comment.
when I pass:
t := 6
i := int64(float64(t)/10 + 0.5)
or
t := float64(6)
i := int64(t/10 + 0.5)
it works
But when I pass:
i := int64(float64(6)/10 + 0.5)
got constant 1.1 truncated to integer
|
also make the linter happy |
6ca5b37 to
385f750
Compare
|
for lint, I should have specified, don't go get gofumt in the fortio directory as that is adding a dependency, please revert go.sum and go.mod changes, and run the formatter also in general avoid force pushing a pr after review has started (unless you accidentally commit unrelated stuff) because it makes it hard for reviewing what changed |
|
unit test? |
4268102 to
385f750
Compare
|
for the build failure: change the comment to be also you can run golangci-lint locally ( |
-reverted and will rerun the formatter, seems that nothing changes after I run linter, the issue is:
test against two branches: master (with fix) and the other (without fix) |
94a59ce to
978cd49
Compare
978cd49 to
4e31d7c
Compare
|
linters seems to have forgotten to show only new issues so merging |
Here is the error log when we run the istio perf benchmark test:
The root cause for this is that according to Golang math/rand source code: https://golang.org/src/math/rand/rand.go (line111) if the seed number is <= 0 it will panic.
Therefore, I set the minimum
ivalue here to be int64(1) [duration type] wheni<=0.cc @ldemailly @mandarjog