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

ggcr: Pass retryBackoff to transport.NewRetry in the remote pkg #1692

Closed
somtochiama opened this issue May 11, 2023 · 5 comments · May be fixed by #1628
Closed

ggcr: Pass retryBackoff to transport.NewRetry in the remote pkg #1692

somtochiama opened this issue May 11, 2023 · 5 comments · May be fixed by #1628
Labels
bug Something isn't working lifecycle/stale

Comments

@somtochiama
Copy link

somtochiama commented May 11, 2023

Describe the feature

Not a bug report but a feature request.

It would be great if the remote.BackOff set in options gets passed now to the created transport in

o.transport = transport.NewRetry(o.transport, transport.WithRetryPredicate(defaultRetryPredicate), transport.WithRetryStatusCodes(o.retryStatusCodes...))
.

This would enable people to configure some parts of the retry operation without providing our own transport.

To Reproduce

Expected behavior

The retry transport is created with the remote.BackOff set in option.

Additional context

Add any other context about the problem here.

  • Version of the module
  • Registry used (e.g., GCR, ECR, Quay)
@somtochiama
Copy link
Author

@jonjohnsonjr @imjasonh Sorry for labelling this as a bug, it's really a question.
Is it a reasonable ask/feature request to have the retry transport configured with remote backoff
e.g

o.transport = transport.NewRetry(o.transport, transport.WithRetryPredicate(defaultRetryPredicate), transport.WithRetryStatusCodes(o.retryStatusCodes...), transport.WithRetryBackoff(o.retryBackoff)))

@imjasonh
Copy link
Collaborator

This seems reasonable to me. It sounds like it might even be an oversight? @jonjohnsonjr does that sound right to you?

@jonjohnsonjr
Copy link
Collaborator

jonjohnsonjr commented May 25, 2023

#1628 (comment)

We can document better what it's for.

@somtochiama
Copy link
Author

@jonjohnsonjr does this mean the remote backoff isn't intended for configuring retries on the transport?

If so, will you consider adding an option that is? Configuring a new transport when you only want to modify a backoff means copying some parts of the code in crane (like the default predicate and some other parts from the internal retry package) and isn't the best experience

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lifecycle/stale
Projects
None yet
3 participants