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

Convert RedisError into nativelink Error #959

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

zbirenbaum
Copy link
Contributor

@zbirenbaum zbirenbaum commented Jun 3, 2024

Description

Adds the ability to convert a RedisError into a Nativelink Error. This will dramatically simplifies error handling when dealing with errors returned by the new redis backend for the scheduler.

Type of change

Please delete options that aren't relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Compiler accepts conversion into nativelink_error::Error from RedisError::into

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Maybe this is a later change, should we have a nativelink-redis package that hosts things coupled to redis? @allada wdyt?

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained

Makes convenience type for stream of action state results accessible
from from the outside functions that will return it. Additionally
changes filtering operations to be an async function so that blocking
will not have to be used when getting the actions to filter is an async
operation.
Adds the ability to convert a RedisError into a Nativelink Error.
This will dramatically simplifies error handling when dealing with
errors returned by the new redis backend for the scheduler.
Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 5 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@zbirenbaum zbirenbaum merged commit cabc0c3 into TraceMachina:main Jun 6, 2024
28 checks passed
@zbirenbaum zbirenbaum deleted the redis-error branch June 6, 2024 16:12
This pull request was closed.
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