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

feat: add multiple attempts options to client.post API #5176

Merged
merged 24 commits into from Sep 27, 2022

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Sep 15, 2022

Goal of this PR is to enable retry_mechanism in the client.

  • Expose max_attempts, initial_backoff, max_backoff and backoff_multiplier in the client.post API
  • Clarify usage of continue_on_error with any kind of Errors
  • Test features
  • Document

Changes

  • Allow to pass max_attempts, initial_backoff, max_backoff and backoff_multiplier to client.post API

    • In gRPC, these are added as configuration to the grpcChannel
    • In HTTP and websocket, we do a manual retry following the formula proposed by grpc
      The initial retry attempt will occur at random(0, initialBackoff). In general, the n-th attempt will occur at random(0, min(initialBackoff*backoffMultiplier**(n-1), maxBackoff)).
  • Document continue_on_error behavior.

@github-actions github-actions bot added size/XS area/core This issue/PR affects the core codebase component/client labels Sep 15, 2022
@JoanFM JoanFM linked an issue Sep 15, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Sep 15, 2022

Codecov Report

Merging #5176 (1231c39) into master (13edf90) will decrease coverage by 0.15%.
The diff coverage is 71.15%.

@@            Coverage Diff             @@
##           master    #5176      +/-   ##
==========================================
- Coverage   84.76%   84.60%   -0.16%     
==========================================
  Files         103      103              
  Lines        7166     7191      +25     
==========================================
+ Hits         6074     6084      +10     
- Misses       1092     1107      +15     
Flag Coverage Δ
jina 84.60% <71.15%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/clients/base/http.py 94.59% <ø> (ø)
jina/clients/base/websocket.py 90.32% <ø> (ø)
jina/clients/mixin.py 95.00% <ø> (ø)
jina/clients/base/helper.py 81.96% <65.21%> (-2.80%) ⬇️
jina/clients/base/grpc.py 85.50% <75.86%> (-3.02%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 94.55% <0.00%> (-1.00%) ⬇️
jina/orchestrate/flow/base.py 83.33% <0.00%> (-0.70%) ⬇️
jina/hubble/hubio.py 90.10% <0.00%> (+0.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot added the area/docs This issue/PR affects the docs label Sep 16, 2022
@JoanFM JoanFM changed the title docs: document continue on error for client feat: retry mechanisms in client Sep 16, 2022
@github-actions github-actions bot added the area/testing This issue/PR affects testing label Sep 22, 2022
@JoanFM JoanFM marked this pull request as ready for review September 23, 2022 09:09
Co-authored-by: AlaeddineAbdessalem <alaeddine-13@live.fr>
samsja
samsja previously requested changes Sep 26, 2022
jina/clients/base/grpc.py Outdated Show resolved Hide resolved
jina/clients/base/helper.py Show resolved Hide resolved
docs/fundamentals/client/client.md Show resolved Hide resolved
@JoanFM JoanFM closed this Sep 27, 2022
@JoanFM JoanFM reopened this Sep 27, 2022
Copy link
Contributor

@JohannesMessner JohannesMessner left a comment

Choose a reason for hiding this comment

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

This seems like a slightly more sophisticated retry mechanism compared to the one that we currently have between gateway and executors.

There (I believe) we don't use the built-in gRPC mechanism, but we do it manually (I think because we want to be able to switch to a different replica when one of them doesn't respond).
Should we implement this exponential backoff strategy there as well?

docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com>
Copy link
Member

@alexcg1 alexcg1 left a comment

Choose a reason for hiding this comment

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

Some clarification needed

docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
Co-authored-by: Alex Cureton-Griffiths <alexcg1@users.noreply.github.com>
alexcg1
alexcg1 previously approved these changes Sep 27, 2022
Copy link
Member

@alexcg1 alexcg1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
docs/fundamentals/client/client.md Outdated Show resolved Hide resolved
Co-authored-by: AlaeddineAbdessalem <alaeddine-13@live.fr>
@github-actions
Copy link

📝 Docs are deployed on https://docs-continue-on-error--jina-docs.netlify.app 🎉

@JoanFM JoanFM merged commit d0838d3 into master Sep 27, 2022
@JoanFM JoanFM deleted the docs-continue-on-error branch September 27, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/docs This issue/PR affects the docs component/client size/M size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document continue_on_error parameter fix: no retries in case of network problems
7 participants