Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Retries to Sync Blocks #3975

Merged
merged 5 commits into from
Mar 30, 2021
Merged

Add Retries to Sync Blocks #3975

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e4acec
Added retries to sync blocks
LeviHarrison Mar 18, 2021
17aeb6a
Add MaxBackoff and MinBackoff
LeviHarrison Mar 19, 2021
5ebd50c
Changed structure of functions and error behavior
LeviHarrison Mar 22, 2021
82f3d78
Changed how errors are collected and returned
LeviHarrison Mar 25, 2021
e23593f
Change error returning
LeviHarrison Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion pkg/storegateway/bucket_stores.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

"github.com/cortexproject/cortex/pkg/storage/bucket"
"github.com/cortexproject/cortex/pkg/storage/tsdb"
"github.com/cortexproject/cortex/pkg/util"
util_log "github.com/cortexproject/cortex/pkg/util/log"
"github.com/cortexproject/cortex/pkg/util/spanlogger"
"github.com/cortexproject/cortex/pkg/util/validation"
Expand Down Expand Up @@ -152,10 +153,31 @@ func (u *BucketStores) InitialSync(ctx context.Context) error {
// SyncBlocks synchronizes the stores state with the Bucket store for every user.
func (u *BucketStores) SyncBlocks(ctx context.Context) error {
return u.syncUsersBlocks(ctx, func(ctx context.Context, s *store.BucketStore) error {
return s.SyncBlocks(ctx)
return u.syncUsersBlocksWithRetries(ctx, s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this design. We pass our own function to u.syncUsersBlocks(). I was expecting something like this:

return u. syncUsersBlocksWithRetries(ctx, func(ctx context.Context, s *store.BucketStore) error {
  return s.SyncBlocks(ctx)
}

And then having syncUsersBlocksWithRetries() calling syncUsersBlocks().

Am I missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I'll get on it!

})
}

func (u *BucketStores) syncUsersBlocksWithRetries(ctx context.Context, s *store.BucketStore) error {
var lastErr error

retries := util.NewBackoff(ctx, util.BackoffConfig{
MinBackoff: 100 * time.Millisecond,
MaxBackoff: 10 * time.Second,
MaxRetries: 3,
Comment on lines +162 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Given that maxRetries is 3, we can never reach max backoff of 10 seconds. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I originally stole that from here and just modified it along the way. Is a 1-second MaxBackoff ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to keep it as is. If you choose to use 1s as MaxBackoff, that's fine too. I think it's more important to do retry here, than how much time it waits.

})

for retries.Ongoing() {
lastErr = s.SyncBlocks(ctx)
if lastErr != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return error on first failure, and will not do any retry. Better version would be:

for retries.Ongoing() {
    err = s.SyncBlocks(ctx)
    if err == nil {
        return nil
    }
    // log error otherwise
    retries.Wait()
}
return retries.Err()

return lastErr
}

retries.Wait()
}

return nil
}

func (u *BucketStores) syncUsersBlocks(ctx context.Context, f func(context.Context, *store.BucketStore) error) (returnErr error) {
defer func(start time.Time) {
u.syncTimes.Observe(time.Since(start).Seconds())
Expand Down