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

Have client retry lost inputs #2600

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Have client retry lost inputs #2600

merged 3 commits into from
Jan 7, 2025

Conversation

rculbertson
Copy link
Contributor

@rculbertson rculbertson commented Dec 3, 2024

Closes SVC-224

Update client to retry inputs if FunctionGetOutputs reports that an input is lost. If an input is lost, FunctionGetOutputs returns a failed output with status GENERIC_STATUS_INTERNAL_FAILURE. The client then retries immediately.


Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


Changelog

@rculbertson rculbertson changed the title Add client retries to client Have client retry lost inputs Dec 3, 2024
@rculbertson rculbertson force-pushed the ryan/remote-lost-inputs branch 2 times, most recently from 076f198 to 33ebf45 Compare December 10, 2024 20:21
@rculbertson rculbertson force-pushed the ryan/remote-lost-inputs branch from 33ebf45 to 974171e Compare December 12, 2024 21:24
@rculbertson rculbertson force-pushed the ryan/remote-lost-inputs branch from 974171e to 53250c3 Compare December 24, 2024 01:16
@rculbertson
Copy link
Contributor Author

@gongy This is now has all the changes we discussed.

modal/exception.py Outdated Show resolved Hide resolved
@rculbertson rculbertson force-pushed the ryan/remote-lost-inputs branch from 7ee0fa7 to da026e4 Compare January 3, 2025 15:24
@rculbertson rculbertson force-pushed the ryan/remote-lost-inputs branch from da026e4 to 07dad3a Compare January 7, 2025 14:53
@rculbertson
Copy link
Contributor Author

@prbot autoapprove

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Approved 👍. @rohansingh will follow-up review this.

@rculbertson rculbertson requested a review from gongy January 7, 2025 16:13
@rculbertson
Copy link
Contributor Author

@gongy I think you already review this last week and gave me a +1 over slack. Mind marking it as approved?

@rculbertson rculbertson merged commit 07e0100 into main Jan 7, 2025
22 checks passed
@rculbertson rculbertson deleted the ryan/remote-lost-inputs branch January 7, 2025 16:45
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