Add a Backofff Handler utils #499

freehan · 2018-09-27T20:32:51Z

Extract the backoff retry logic from batchSyncer and write 2 util package that can be reused by different syncers, plus unit testing:

backoffHandler calculates exponential backoff delays
backoffRetryHandler triggers retryFunc after back off delay.

These utils are building blocks for the new transactionSyncer

cc: @agau4779

bowei · 2018-09-27T21:14:38Z

pkg/neg/syncers/backoff.go

+var MaxRetryError = fmt.Errorf("maximum retry exceeded")
+
+// backoffHanlder handles delays for back off retry
+type backoffHanlder interface {


bowei · 2018-09-27T21:15:17Z

pkg/neg/syncers/backoff.go

+}
+
+// boundedExponentialBackendOffHandler is a back off handler that implements bounded exponential back off.
+type boundedExponentialBackendOffHandler struct {


"bounded" is probably extraneous (I would think every implementation has a bound)

bowei · 2018-09-27T21:15:44Z

pkg/neg/syncers/backoff.go

+	return handler.lastRetryDelay, nil
+}
+
+func (handler *boundedExponentialBackendOffHandler) ResetRetryDelay() {


why not just create a new one instead of reset?

bowei · 2018-10-14T23:28:57Z

pkg/neg/syncers/backoff.go

+	"time"
+)
+
+var MaxRetryError = fmt.Errorf("maximum retry exceeded")


ErrRetriesExceeded

bowei · 2018-10-14T23:29:36Z

pkg/neg/syncers/backoff.go

+	// NextRetryDelay returns the delay for next retry or error if maximum number of retries exceeded.
+	NextRetryDelay() (time.Duration, error)
+	// ResetRetryDelay resets the retry delay
+	ResetRetryDelay()


Alternative: construct a new backoffHandler instead of reset

That basically push more load on Golang GC. Imagine I need to create a new backoffHandler object after every successful NEG sync.

This is usually not the case for a modern GC (efficiency issues due to GC), unless you are creating an incredible # of objects.

Not having reset() usually makes it easier to reason about the object.

Imagine there are multiple go routines running NEG API calls. And when they finished succesfully, every one will call Reset. Or, they need to create duplicated objects.

bowei · 2018-10-14T23:30:12Z

pkg/neg/syncers/backoff.go

+}
+
+// NextRetryDelay returns the next back off delay for retry.
+func (handler *boundedExponentialBackOffHandler) NextRetryDelay() (time.Duration, error) {


We should probably add jitter. Does the code vendored in from client-go offer any resuable bits?

Added jitter.

freehan · 2018-10-15T21:06:29Z

Fixed the comments. Ready for review

agau4779

Do the tests pass if we run them with -parallel ?

agau4779 · 2018-10-26T18:35:43Z

pkg/neg/syncers/backoff.go

+	handler.lock.Lock()
+	defer handler.lock.Unlock()
+	handler.retryCount += 1
+	if handler.maxRetries > 0 && handler.retryCount > handler.maxRetries {


nit - don't need the > 0

> 0 is for the case where one do not want to have a maxRetries limit

agau4779 · 2018-10-26T18:38:29Z

pkg/neg/syncers/retry_handler_test.go

+
+func TestStartAndStopBackoffRetryHandler(t *testing.T) {
+	helper := &retryHandlerTestHelper{}
+	handler := NewDelayRetryHandler(helper.incrementCount, NewExponentialBackendOffHandler(15, testMinRetryDelay, testMaxRetryDelay))


This and line 88 - could we put the max retries in a variable at the top? const testMaxRetries = 10?

pkg/neg/syncers/retry_handler.go

MrHohn · 2018-10-26T20:10:32Z

pkg/neg/syncers/retry_handler.go

+	retryFunc func()
+}
+
+func NewDelayRetryHandler(retryFunc func(), backoff backoffHandler) *backoffRetryHandler {


To make sure I understand it correctly:

Each NEG can get one retry handler.

handler.Run() starts before any NE operation for this NEG is made.

Each NEG can trigger multiple go routines that handle NE operations in parallel. Some of them may fail at different time.

All failed operations will attempt to call handler.Retry(). Conceptually all calls to handler.Retry() will be merged into one until the next retry runs.

Each retryFunc() can trigger go routines that execute NE operations necessary for the entire NEG based on its current state.

Yes.

One more thing, For successful operations, they will call Reset().

If some are successful and some failed, the successful operations will not be retried. Eventually, only the failed ones will be retries plus back off exponentially.

pkg/neg/syncers/retry_handler.go

freehan · 2018-10-31T00:25:42Z

Ready for another round

MrHohn · 2018-10-31T21:02:13Z

The retry handler part LGTM.

freehan · 2018-11-02T22:10:46Z

Can I get this merged?

freehan · 2018-11-05T19:49:34Z

Ping

MrHohn · 2018-11-08T21:01:40Z

/lgtm

k8s-ci-robot · 2018-11-08T21:01:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, MrHohn

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:

~~OWNERS~~ [MrHohn,freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

freehan assigned bowei Sep 27, 2018

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 27, 2018

k8s-ci-robot requested review from nikhiljindal and thockin September 27, 2018 20:32

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 27, 2018

freehan force-pushed the backoff branch 3 times, most recently from fbe0648 to 5b73089 Compare October 2, 2018 17:30

freehan mentioned this pull request Oct 2, 2018

Add Syncer Skeleton #509

Merged

freehan force-pushed the backoff branch from 5b73089 to eea11cd Compare October 3, 2018 21:15

bowei reviewed Oct 14, 2018

View reviewed changes

freehan force-pushed the backoff branch 2 times, most recently from 79b87b7 to c3f0147 Compare October 15, 2018 21:06

add backoff handler for exponential backoff

45022ff

freehan force-pushed the backoff branch from c3f0147 to 45022ff Compare October 15, 2018 21:10

freehan assigned MrHohn Oct 20, 2018

agau4779 reviewed Oct 26, 2018

View reviewed changes

MrHohn reviewed Oct 26, 2018

View reviewed changes

add backoff retry handler

0b6dd36

freehan force-pushed the backoff branch from 6ebc7dd to 0b6dd36 Compare October 31, 2018 00:25

freehan mentioned this pull request Nov 2, 2018

Transaction Syncer #532

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2018

k8s-ci-robot merged commit 411b66d into kubernetes:master Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Backofff Handler utils #499

Add a Backofff Handler utils #499

freehan commented Sep 27, 2018 •

edited

Loading

bowei Sep 27, 2018

bowei Sep 27, 2018

bowei Sep 27, 2018

bowei Oct 14, 2018

bowei Oct 14, 2018

freehan Oct 15, 2018

bowei Oct 27, 2018

bowei Oct 27, 2018

freehan Oct 29, 2018

bowei Oct 14, 2018

freehan Oct 31, 2018

freehan commented Oct 15, 2018

agau4779 left a comment

agau4779 Oct 26, 2018

freehan Oct 29, 2018

agau4779 Oct 26, 2018

freehan Oct 29, 2018

MrHohn Oct 26, 2018 •

edited

Loading

freehan Oct 29, 2018

freehan commented Oct 31, 2018

MrHohn commented Oct 31, 2018

freehan commented Nov 2, 2018

freehan commented Nov 5, 2018

MrHohn commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

Add a Backofff Handler utils #499

Add a Backofff Handler utils #499

Conversation

freehan commented Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Oct 15, 2018

agau4779 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Oct 31, 2018

MrHohn commented Oct 31, 2018

freehan commented Nov 2, 2018

freehan commented Nov 5, 2018

MrHohn commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

freehan commented Sep 27, 2018 •

edited

Loading

MrHohn Oct 26, 2018 •

edited

Loading