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.

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

[PE-1779] add retry in case of inserter failure #28

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

avivl
Copy link

@avivl avivl commented Apr 8, 2024

Add retry for k8s calls in inserter

@avivl avivl requested review from spenrose and smintz April 8, 2024 15:59
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.97%. Comparing base (baa447d) to head (423b399).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #28   +/-   ##
=======================================
  Coverage   70.97%   70.97%           
=======================================
  Files          15       15           
  Lines        1664     1664           
=======================================
  Hits         1181     1181           
  Misses        346      346           
  Partials      137      137           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avivl avivl force-pushed the feature/PE-1779-add-retry-in-case-of-inserter-failure branch from f882e13 to 4ecf612 Compare April 8, 2024 20:32
Copy link

@smintz smintz left a comment

Choose a reason for hiding this comment

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

I am not familiar with this retry mechanism, so I might be wrong on my assumptions, but, from what I noticed:

  • There is no limit on how many retries
  • There is no indication or log something happened
  • There is no validation for the type of error (should we retry if context canceled?)

This means if something is wrong, this will go to endless loop without any indication to the caller.
I think its better to move the retry mechanism to the inserter/agent and not to implement it on the KV level.
thoughts?
I am usually using this library: https://pkg.go.dev/github.com/avast/retry-go

@avivl
Copy link
Author

avivl commented Apr 9, 2024

I am not familiar with this retry mechanism, so I might be wrong on my assumptions, but, from what I noticed:

  • There is no limit on how many retries
  • There is no indication or log something happened
  • There is no validation for the type of error (should we retry if context canceled?)

This means if something is wrong, this will go to endless loop without any indication to the caller. I think its better to move the retry mechanism to the inserter/agent and not to implement it on the KV level. thoughts? I am usually using this library: https://pkg.go.dev/github.com/avast/retry-go

  • There is no limit on how many retries - var DefaultRetry = wait.Backoff{ Steps: 5, Duration: 10 * time.Millisecond, Factor: 1.0, Jitter: 0.1, }

  • I'll add logs

  • There is no validation for the type of error (should we retry if context canceled?) this is only for errors returned from k8s.

I prefer to use k8s retry and not an external library

@avivl avivl force-pushed the feature/PE-1779-add-retry-in-case-of-inserter-failure branch from 4ecf612 to 2b17ad2 Compare April 9, 2024 13:51
Copy link

@smintz smintz left a comment

Choose a reason for hiding this comment

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

few nits

agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
agent/configmaps/configmaps.go Outdated Show resolved Hide resolved
@avivl avivl force-pushed the feature/PE-1779-add-retry-in-case-of-inserter-failure branch from 2b17ad2 to 423b399 Compare April 9, 2024 14:53
@avivl avivl merged commit fee3624 into main Apr 10, 2024
7 checks passed
@avivl avivl deleted the feature/PE-1779-add-retry-in-case-of-inserter-failure branch April 10, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants