You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/01/26 15:41:42 UTC
[GitHub] [pulsar-client-go] wuYin opened a new pull request #454: Make backoff policy configurable and use uniformly
wuYin opened a new pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454
### Motivation
For now, client backoff interval is hardcoded to start from 100s and increasing exponentially to 60s.
In some case, client need reconnect to restarted broker as soon as possible, rather than still waiting while broker is available.
Like this one, during the 51.2s backoff interval, broker already restarted and available.
### Modifications
- Modular backoff policy and make it configurable, retained mandatory stop policy which added in [pulsar#747](https://github.com/apache/pulsar/pull/747)
- Added `backoff_test.go` to test backoff's functionality
- Added 2 backoff configurations for Client and use uniformly in producer, consumer reconnection
```go
// Set the initial duration of time for a backoff interval (default: 100ms)
InitBackoff time.Duration
// Set the maximum duration of time for a backoff interval (default: 60s)
MaxBackoff time.Duration
```
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Request help review if you have time, thanks.
Now I added 2 thresholds for backoff (I think threshold is 1% of the default is reasonable)
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Help review if you have time, thanks.
Now I added 2 thresholds for backoff (I think threshold is 1% of the default is reasonable)
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Help review if you have time, thanks a lot.
Now I added 2 thresholds for backoff
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564615545
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If mandatoryStop set to 0, this always be true in first time, means there is no mandatory stop which is on expected.
So I rule it out in above `if b.mandatoryStop > 0`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin removed a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin removed a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596751
@merlimat
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564615545
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If mandatoryStop set to 0, this always be true in first time, means there is no mandatory stop which is on expected.
So I rule it out in above `if b.mandatoryStop > 0`, more readable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin removed a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin removed a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767697458
@merlimat Help review if you have time, thanks a lot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767697458
@merlimat Help review if you have time, thanks a lot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Request help review if you have time, thanks.
I added 2 thresholds for backoff (I think threshold is 1% of the default may reasonable)
- `minInitBackoff` : 1ms
- `minMaxBackoff` : 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Request help review if you have time, thanks.
Now I added 2 thresholds for backoff (I think threshold is 1% of the default may reasonable)
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596751
@merlimat
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin closed pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin closed pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767707216
> The problem I see with this change is that it allows misconfigured clients that retry too quickly, to overwhelm brokers with a flood of requests.
Thanks for review.
I understand this concern, for now backoff intervals kept default in `[100ms, ..., 60s]`
I assumed user knows what they're doing, for me, I may need reduce `MaxBackoffInterval` to 20s for reducing recovery time.
Should I add a WARNING log if `MaxBackoffInterval` less than 1s, or something else?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin removed a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin removed a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596751
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596923
@merlimat
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Request help review if you have time, thanks.
I added 2 thresholds for backoff (I think threshold is 1% of the default may reasonable)
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Help review if you have time, thanks.
Now I added 2 thresholds for backoff
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596751
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771597799
@merlimat
If this features is intentionally restrained to avoid the risk of misconfiguration, it's ok to close this PR, I'll do stricter timeout checking in my application.
Thanks for the review and looking forward to reply.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wolfstudy commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-775617205
> @merlimat
> If this features is intentionally restrained to avoid the risk of misconfiguration, it's ok to close this PR, I'll do stricter timeout checking in my application.
> Thanks for the review and looking forward to reply.
@wuYin Close this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564615545
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If mandatoryStop set to 0, this always be true in first time, means there is no mandatory stop which is on expected.
So I rule it out in above `if b.mandatoryStop > 0`, more readable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin removed a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin removed a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767707216
> The problem I see with this change is that it allows misconfigured clients that retry too quickly, to overwhelm brokers with a flood of requests.
Thanks for review.
I understand this concern, for now backoff intervals kept default in `[100ms, ..., 60s]`
I assumed user knows what they're doing, for me, I may need reduce `MaxBackoffInterval` to 20s for reducing recovery time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin removed a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin removed a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-771596923
@merlimat
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767919004
@merlimat Help review if you have time, thanks.
Now I added 2 thresholds for backoff (I think threshold is the default value of 1% is reasonable)
- `minInitBackoff` which is 1ms
- `minMaxBackoff` which is 1sec
If user-provided backoff config less than these, create client will be failed with a invalid config error, checked by `TestMisconfiguredBackoff`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin edited a comment on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin edited a comment on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767707216
> The problem I see with this change is that it allows misconfigured clients that retry too quickly, to overwhelm brokers with a flood of requests.
Thanks for review.
I understand this concern, for now backoff intervals kept default in `[100ms, ..., 60s]`
I assumed user knows what they're doing, for me, I may need reduce `MaxBackoffInterval` to 20s for reducing recovery time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564676218
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If set `mandatoryStop` to 0, `tried+curGap > 0` always true, means mandatory stop is invalid and is expected, so I added above `if b.mandatoryStop > 0`, more readable
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] merlimat commented on pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#issuecomment-767697481
The problem I see with this change is that it allows misconfigured clients that retry too quickly, to overwhelm brokers with a flood of requests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564676218
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If set `mandatoryStop` to 0, this always be true for the ONLY one mandatory stop, means mandatory stop take no effective which is on expected, so I added above `if b.mandatoryStop > 0`, more readable
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wuYin commented on a change in pull request #454: Make backoff policy configurable and use uniformly
Posted by GitBox <gi...@apache.org>.
wuYin commented on a change in pull request #454:
URL: https://github.com/apache/pulsar-client-go/pull/454#discussion_r564676218
##########
File path: pulsar/internal/backoff.go
##########
@@ -18,28 +18,64 @@
package internal
import (
+ "math/rand"
"time"
)
-// Backoff
+func init() {
+ rand.Seed(time.Now().UnixNano())
+}
+
type Backoff struct {
- backoff time.Duration
+ initGap, nextGap, maxGap time.Duration
+
+ firstRetry time.Time
+ mandatoryStop time.Duration
+ mandatoryStopped bool
}
-const (
- minBackoff = 100 * time.Millisecond
- maxBackoff = 60 * time.Second
-)
+func NewBackoff(initGap, maxGap, mandatoryStop time.Duration) *Backoff {
+ return &Backoff{
+ initGap: initGap,
+ nextGap: initGap,
+ maxGap: maxGap,
+ firstRetry: time.Time{},
+ mandatoryStop: mandatoryStop,
+ mandatoryStopped: false,
+ }
+}
+
+// for test mock convenience
+type nowTimeProvider func() time.Time
+
+var systemNow nowTimeProvider = func() time.Time {
+ return time.Now()
+}
-// Next
func (b *Backoff) Next() time.Duration {
- // Double the delay each time
- b.backoff += b.backoff
- if b.backoff.Nanoseconds() < minBackoff.Nanoseconds() {
- b.backoff = minBackoff
- } else if b.backoff.Nanoseconds() > maxBackoff.Nanoseconds() {
- b.backoff = maxBackoff
+ curGap := b.nextGap
+ if curGap < b.maxGap {
+ b.nextGap = MinDuration(2*b.nextGap, b.maxGap)
}
- return b.backoff
+ if b.mandatoryStop > 0 && !b.mandatoryStopped {
+ now := systemNow()
+ tried := time.Duration(0)
+ if curGap == b.initGap {
+ b.firstRetry = now
+ } else {
+ tried = now.Sub(b.firstRetry)
+ }
+ if tried+curGap > b.mandatoryStop {
+ // reached mandatory stop, reset current retry gap
Review comment:
If mandatoryStop set to 0, this always be true in first time, means there is no mandatory stop which is on expected.
So I rule it out in above `if b.mandatoryStop > 0`, more readable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org