Repository: snowflakedb/gosnowflake
Pull request #1051 (2024-02-15 09:19:49)

SNOW-991467: Fix retry backoff period for non-auth requests
### Description
issue 804: https://github.com/snowflakedb/snowflake-sdks-drivers-issues-teamwork/issues/804

This PR fixes the retry backoff period for query requests. 
- changed the base wait time from 5s to 1s
- changed the max wait time from 160s 16s
- The implementation now matches the JDBC driver's [decorrelated jitter](https://github.com/snowflakedb/snowflake-jdbc/blob/master/src/main/java/net/snowflake/client/util/DecorrelatedJitterBackoff.java#L19C15-L19C28)


### Checklist
- [x] Code compiles correctly
- [x] Run ``make fmt`` to fix inconsistent formats
- [x] Run ``make lint`` to get lint errors and fix all of them
- [x] Created tests which fail without the change (if possible)
- [x] All tests passing
- [x] Extended the README / documentation, if necessary


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/1051?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`bd9aa25`)](https://app.codecov.io/gh/snowflakedb/gosnowflake/commit/bd9aa25678ae6e0d78364abd5074e54b9993eb52?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) 84.09% compared to head [(`7cafae9`)](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/1051?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb) 82.53%.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   84.09%   82.53%   -1.57%     
==========================================
  Files          50       49       -1     
  Lines        9878     9876       -2     
==========================================
- Hits         8307     8151     -156     
- Misses       1571     1725     +154     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/snowflakedb/gosnowflake/pull/1051?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=snowflakedb).


Patch:
diff --git a/doc.go b/doc.go
index 1ebb470..e1c4042 100644
--- a/doc.go
+++ b/doc.go
@@ -90,6 +90,10 @@ The following connection parameters are supported:
     is 60 seconds. The login request gives up after the timeout length if the
     HTTP response is success.
 
+  - requestTimeout: Specifies the timeout, in seconds, for a query to complete.
+    0 (zero) specifies that the driver should wait indefinitely. The default is 0 seconds.
+    The query request gives up after the timeout length if the HTTP response is success.
+
   - authenticator: Specifies the authenticator to use for authenticating user credentials:
 
   - To use the internal Snowflake authenticator, specify snowflake (Default).
diff --git a/retry.go b/retry.go
index 5292f62..80ceb56 100644
--- a/retry.go
+++ b/retry.go
@@ -40,7 +40,8 @@ var clientErrorsStatusCodesEligibleForRetry = []int{
 
 func init() {
 	random = rand.New(rand.NewSource(time.Now().UnixNano()))
-	defaultWaitAlgo = &waitAlgo{mutex: &sync.Mutex{}, random: random, base: 5 * time.Second, cap: 160 * time.Second}
+	// sleep time before retrying starts from 1s and the max sleep time is 16s
+	defaultWaitAlgo = &waitAlgo{mutex: &sync.Mutex{}, random: random, base: 1 * time.Second, cap: 16 * time.Second}
 }
 
 const (
@@ -211,21 +212,19 @@ func (w *waitAlgo) calculateWaitBeforeRetryForAuthRequest(attempt int, currWaitT
 	return time.Duration(jitteredSleepTime * float64(time.Second))
 }
 
-func (w *waitAlgo) calculateWaitBeforeRetry(attempt int, sleep time.Duration) time.Duration {
+func (w *waitAlgo) calculateWaitBeforeRetry(sleep time.Duration) time.Duration {
 	w.mutex.Lock()
 	defer w.mutex.Unlock()
-	t := 3*sleep - w.base
-	switch {
-	case t > 0:
-		return durationMin(w.cap, randSecondDuration(t)+w.base)
-	case t < 0:
-		return durationMin(w.cap, randSecondDuration(-t)+3*sleep)
-	}
-	return w.base
+	// use decorrelated jitter in retry time
+	randDuration := randMilliSecondDuration(w.base, sleep*3)
+	return durationMin(w.cap, randDuration)
 }
 
-func randSecondDuration(n time.Duration) time.Duration {
-	return time.Duration(random.Int63n(int64(n/time.Second))) * time.Second
+func randMilliSecondDuration(base time.Duration, bound time.Duration) time.Duration {
+	baseNumber := int64(base / time.Millisecond)
+	boundNumber := int64(bound / time.Millisecond)
+	randomDuration := random.Int63n(boundNumber-baseNumber) + baseNumber
+	return time.Duration(randomDuration) * time.Millisecond
 }
 
 func (w *waitAlgo) getJitter(currWaitTime float64) float64 {
@@ -342,7 +341,7 @@ func (r *retryHTTP) execute() (res *http.Response, err error) {
 		if isLoginRequest(req) {
 			sleepTime = defaultWaitAlgo.calculateWaitBeforeRetryForAuthRequest(retryCounter, sleepTime)
 		} else {
-			sleepTime = defaultWaitAlgo.calculateWaitBeforeRetry(retryCounter, sleepTime)
+			sleepTime = defaultWaitAlgo.calculateWaitBeforeRetry(sleepTime)
 		}
 
 		if totalTimeout > 0 {


Test patch:
diff --git a/assert_test.go b/assert_test.go
index 91a69c3..3f47226 100644
--- a/assert_test.go
+++ b/assert_test.go
@@ -58,6 +58,10 @@ func assertBetweenE(t *testing.T, value float64, min float64, max float64, descr
 	errorOnNonEmpty(t, validateValueBetween(value, min, max, descriptions...))
 }
 
+func assertBetweenInclusiveE(t *testing.T, value float64, min float64, max float64, descriptions ...string) {
+	errorOnNonEmpty(t, validateValueBetweenInclusive(value, min, max, descriptions...))
+}
+
 func assertEmptyE[T any](t *testing.T, actual []T, descriptions ...string) {
 	errorOnNonEmpty(t, validateEmpty(actual, descriptions...))
 }
@@ -126,6 +130,14 @@ func validateValueBetween(value float64, min float64, max float64, descriptions
 	return fmt.Sprintf("expected \"%f\" should be between \"%f\" and  \"%f\" but did not. %s", value, min, max, desc)
 }
 
+func validateValueBetweenInclusive(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\" inclusively but did not. %s", value, min, max, desc)
+}
+
 func validateEmpty[T any](value []T, descriptions ...string) string {
 	if len(value) == 0 {
 		return ""
diff --git a/retry_test.go b/retry_test.go
index aeb9673..9208bdd 100644
--- a/retry_test.go
+++ b/retry_test.go
@@ -606,3 +606,41 @@ func TestCalculateRetryWait(t *testing.T) {
 		})
 	}
 }
+
+func TestCalculateRetryWaitForNonAuthRequests(t *testing.T) {
+	// test for randomly selected currWaitTime values
+	// maxSleepTime is the limit value
+	tcs := []struct {
+		currWaitTime float64
+		maxSleepTime float64
+	}{
+		{
+			currWaitTime: 3.346609,
+			maxSleepTime: 10.039827,
+		},
+		{
+			currWaitTime: 4.260357,
+			maxSleepTime: 12.781071,
+		},
+		{
+			currWaitTime: 5.154231,
+			maxSleepTime: 15.462693,
+		},
+		{
+			currWaitTime: 7.249255,
+			maxSleepTime: 16,
+		},
+		{
+			currWaitTime: 23.598257,
+			maxSleepTime: 16,
+		},
+	}
+
+	for _, tc := range tcs {
+		defaultMinSleepTime := 1
+		t.Run(fmt.Sprintf("currWaitTime: %v", tc.currWaitTime), func(t *testing.T) {
+			result := defaultWaitAlgo.calculateWaitBeforeRetry(time.Duration(tc.currWaitTime) * time.Second)
+			assertBetweenInclusiveE(t, result.Seconds(), float64(defaultMinSleepTime), tc.maxSleepTime)
+		})
+	}
+}