Repository: snowflakedb/gosnowflake
Pull request #956 (2023-11-06 07:57:19)

SNOW-878073 Fix of retry algorithm
### Description
Please explain the changes you made here.

### Checklist
- [ ] Code compiles correctly
- [ ] Run ``make fmt`` to fix inconsistent formats
- [ ] Run ``make lint`` to get lint errors and fix all of them
- [ ] Created tests which fail without the change (if possible)
- [ ] All tests passing
- [ ] Extended the README / documentation, if necessary


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/956?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) Report
> Merging [#956](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/956?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) (eeac8ae) into [master](https://app.codecov.io/gh/snowflakedb/gosnowflake/commit/56c36182c14bfb53168a40e8557646afdef6daa9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) (56c3618) will **decrease** coverage by `0.63%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master     #956      +/-   ##
==========================================
- Coverage   83.80%   83.17%   -0.63%     
==========================================
  Files          50       49       -1     
  Lines        9556     9549       -7     
==========================================
- Hits         8008     7942      -66     
- Misses       1548     1607      +59     
```


| [Files](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/956?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) | Coverage Δ | |
|---|---|---|
| [retry.go](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/956?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb#diff-cmV0cnkuZ28=) | `93.53% <100.00%> (-0.16%)` | :arrow_down: |

... and [5 files with indirect coverage changes](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/956/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb)

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!

sfc-gh-pmotacki: I have read the CLA Document and I hereby sign the CLA

Patch:
diff --git a/retry.go b/retry.go
index 8995b9c..e46148f 100644
--- a/retry.go
+++ b/retry.go
@@ -208,26 +208,17 @@ func isQueryRequest(url *url.URL) bool {
 func (w *waitAlgo) calculateWaitBeforeRetry(attempt int, currWaitTime float64) float64 {
 	w.mutex.Lock()
 	defer w.mutex.Unlock()
-	var jitterPercentage = 0.5
-	if attempt < 2 {
-		jitterPercentage = 0.25 // to ensure there will be sleep time increase between attempts
-	}
-	jitterAmount := w.getJitter(currWaitTime, jitterPercentage)
-	jitteredSleepTime := math.Pow(2, float64(attempt)) + jitterAmount
+	jitterAmount := w.getJitter(currWaitTime)
+	jitteredSleepTime := chooseRandomFromRange(currWaitTime+jitterAmount, math.Pow(2, float64(attempt))+jitterAmount)
 	return jitteredSleepTime
 }
 
-func (w *waitAlgo) getJitter(currWaitTime float64, jitterPercentage float64) float64 {
-	multiplicationFactor := chooseRandomFromValues(w.random, []int{-1, 1}) // random int from [-1, 1]
-	jitterAmount := jitterPercentage * currWaitTime * float64(multiplicationFactor)
+func (w *waitAlgo) getJitter(currWaitTime float64) float64 {
+	multiplicationFactor := chooseRandomFromRange(-1, 1)
+	jitterAmount := 0.5 * currWaitTime * multiplicationFactor
 	return jitterAmount
 }
 
-func chooseRandomFromValues[T any](random *rand.Rand, arr []T) T {
-	valIdx := random.Intn(len(arr))
-	return arr[valIdx]
-}
-
 type requestFunc func(method, urlStr string, body io.Reader) (*http.Request, error)
 
 type clientInterface interface {
diff --git a/util.go b/util.go
index 1e07bb3..32df50e 100644
--- a/util.go
+++ b/util.go
@@ -7,6 +7,7 @@ import (
 	"database/sql/driver"
 	"fmt"
 	"io"
+	"math/rand"
 	"os"
 	"strings"
 	"sync"
@@ -267,3 +268,7 @@ func contains[T comparable](s []T, e T) bool {
 	}
 	return false
 }
+
+func chooseRandomFromRange(min float64, max float64) float64 {
+	return rand.Float64()*(max-min) + min
+}


Test patch:
diff --git a/assert_test.go b/assert_test.go
index d25217b..37b4733 100644
--- a/assert_test.go
+++ b/assert_test.go
@@ -42,6 +42,10 @@ func assertHasPrefixE(t *testing.T, actual string, expectedPrefix string, descri
 	errorOnNonEmpty(t, validateHasPrefix(actual, expectedPrefix, descriptions...))
 }
 
+func assertBetweenE(t *testing.T, value float64, min float64, max float64, descriptions ...string) {
+	errorOnNonEmpty(t, validateValueBetween(value, min, max, descriptions...))
+}
+
 func fatalOnNonEmpty(t *testing.T, errMsg string) {
 	if errMsg != "" {
 		t.Fatal(formatErrorMessage(errMsg))
@@ -98,6 +102,14 @@ func validateHasPrefix(actual string, expectedPrefix string, descriptions ...str
 	return fmt.Sprintf("expected \"%s\" to start with \"%s\" but did not. %s", actual, expectedPrefix, desc)
 }
 
+func validateValueBetween(value float64, min float64, max float64, descriptions ...string) string {
+	if value > min && value < max {
+		return ""
+	}
+	desc := joinDescriptions(descriptions...)
+	return fmt.Sprintf("expected \"%f\" should be between \"%f\" and  \"%f\" but did not. %s", value, min, max, desc)
+}
+
 func joinDescriptions(descriptions ...string) string {
 	return strings.Join(descriptions, " ")
 }
diff --git a/retry_test.go b/retry_test.go
index 5815d66..90c77de 100644
--- a/retry_test.go
+++ b/retry_test.go
@@ -7,7 +7,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -533,18 +532,81 @@ func TestIsRetryable(t *testing.T) {
 	}
 }
 
-func TestExponentialJitterBackoff(t *testing.T) {
-	retryTimes := make([]float64, 10)
-	inputTime := 1.0
-	for i := 0; i < 10; i++ {
-		resultTime := defaultWaitAlgo.calculateWaitBeforeRetry(i+1, inputTime)
-		retryTimes[i] = resultTime
-		inputTime = resultTime
+func TestCalculateRetryWait(t *testing.T) {
+	// test for randomly selected attempt and currWaitTime values
+	// minSleepTime, maxSleepTime are limit values
+	tcs := []struct {
+		attempt      int
+		currWaitTime float64
+		minSleepTime float64
+		maxSleepTime float64
+	}{
+		{
+			attempt:      1,
+			currWaitTime: 3.346609,
+			minSleepTime: 0.326695,
+			maxSleepTime: 5.019914,
+		},
+		{
+			attempt:      2,
+			currWaitTime: 4.260357,
+			minSleepTime: 1.869821,
+			maxSleepTime: 6.390536,
+		},
+		{
+			attempt:      3,
+			currWaitTime: 7.857728,
+			minSleepTime: 3.928864,
+			maxSleepTime: 11.928864,
+		},
+		{
+			attempt:      4,
+			currWaitTime: 7.249255,
+			minSleepTime: 3.624628,
+			maxSleepTime: 19.624628,
+		},
+		{
+			attempt:      5,
+			currWaitTime: 23.598257,
+			minSleepTime: 11.799129,
+			maxSleepTime: 43.799129,
+		},
+		{
+			attempt:      8,
+			currWaitTime: 27.088613,
+			minSleepTime: 13.544306,
+			maxSleepTime: 269.544306,
+		},
+		{
+			attempt:      10,
+			currWaitTime: 30.879329,
+			minSleepTime: 15.439664,
+			maxSleepTime: 1039.439664,
+		},
+		{
+			attempt:      12,
+			currWaitTime: 39.919798,
+			minSleepTime: 19.959899,
+			maxSleepTime: 4115.959899,
+		},
+		{
+			attempt:      15,
+			currWaitTime: 33.750758,
+			minSleepTime: 16.875379,
+			maxSleepTime: 32784.875379,
+		},
+		{
+			attempt:      20,
+			currWaitTime: 32.357793,
+			minSleepTime: 16.178897,
+			maxSleepTime: 1048592.178897,
+		},
 	}
 
-	for i := 0; i < 9; i++ {
-		if retryTimes[i] >= retryTimes[i+1] {
-			log.Fatalf("expected consequent values to be greater than previous ones; array: %v", retryTimes)
-		}
+	for _, tc := range tcs {
+		t.Run(fmt.Sprintf("attmept: %v", tc.attempt), func(t *testing.T) {
+			result := defaultWaitAlgo.calculateWaitBeforeRetry(tc.attempt, tc.currWaitTime)
+			assertBetweenE(t, result, tc.minSleepTime, tc.maxSleepTime)
+		})
 	}
 }