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

Don't remove Error context. #199

Merged

Conversation

chrisstaite-menlo
Copy link
Collaborator

@chrisstaite-menlo chrisstaite-menlo commented Jul 17, 2023

Unexplicably, the Error context was removed when getting the upstream data from a GrpcStore. This means that the Code::NotFound is removed and copious error messages are spewed for missing action cache entries.

Resolves #197


This change is Reviewable

Unexplicably, the Error context was removed when getting the upstream data from
a GrpcStore.  This means that the Code::NotFound is removed and copious error
messages are spewed for missing action cache entries.

Resolves TraceMachina#197
@chrisstaite-menlo
Copy link
Collaborator Author

I'd like to write a test for this, but there are no tests for GrpcStore currently and my previous attempt at writing a test for a Grpc upstream ended up pulling in a lot of dependencies to get it to connect and it was requested to be removed, so I'm leaving this without a regression test unless advised otherwise.

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.

Uuug, writing a test for this is going to suck. Can we make a ticket for this to write a regression test for this? This probably needs to be figured out for grpc_scheduler too.
:lgtm:

Reviewable status: 0 of 1 files reviewed, all discussions resolved

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.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @chrisstaite-menlo)

@chrisstaite-menlo
Copy link
Collaborator Author

Have raised #200

@chrisstaite-menlo chrisstaite-menlo merged commit e9ab61e into TraceMachina:master Jul 17, 2023
6 checks passed
@chrisstaite-menlo chrisstaite-menlo deleted the fix_no_action_result branch September 8, 2023 08:05
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.

Wrapping GrpcStore with FastSlowStore failure for AC
2 participants