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

Cleanup errors #88

Merged

Conversation

hannahhoward
Copy link
Collaborator

Goals

  • Use more standardized errors
  • Insure 1 error per failing request (don't error twice on a response rejection)

Implementation

  • add some sentinel errors
  • in OnRequestCompleted, check if the request is already failing before dispatching an incomplete error
  • add a test to verify this behavior

@hannahhoward hannahhoward changed the base branch from master to feat/add-bool-to-revalidator-interfaces September 28, 2020 08:25
Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

We need a test case for ErrIncomplete .

LGTM otherwise.

for testCase, isPull := range testCases {
t.Run(testCase, func(t *testing.T) {
// ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
// defer cancel()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: dead code.

}
// send an error, but only if we haven't already errored for some reason
if chst.Status() != datatransfer.Failing && chst.Status() != datatransfer.Failed {
return m.channels.Error(chid, datatransfer.ErrIncomplete)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hannahhoward There's no test for ErrIncomplete.

@hannahhoward hannahhoward force-pushed the feat/add-bool-to-revalidator-interfaces branch from f7d85bd to a400317 Compare September 29, 2020 09:52
… for a need to revalidate (#87)

* feat(revalidator): add handled bool

Previously, the revalidators were very ambiguous in identifying if they had actually handled a
request for a revalidation check or not -- a "nil, nil" or "voucher result, nil" was interpreted as
not being handled. This is unfortunate, cause particularly a "voucher result, nil" is an indication
the request is handled by this check, and we should stop processing other revalidators that might
override the value with "nil, nil". We now add a boolean to disambiguate whether the revalidator
"recognized" this request --- if true, processing stops.

* style(comments): fix types
cleanup errors -- move more error codes to proper types and insure single error dispatched
@hannahhoward hannahhoward merged commit fff106b into feat/add-bool-to-revalidator-interfaces Sep 29, 2020
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.

2 participants