-
Notifications
You must be signed in to change notification settings - Fork 296
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
indexers: update indexer error types. #2770
Conversation
f9a75e6
to
f589ae3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but are you sure about returning in the switch?
if err != nil { | ||
log.Errorf("%s: unable to disconnect block: %v", idx.Name(), err) | ||
msg := fmt.Sprintf("%s: unable to disconnect block: %v", | ||
idx.Name(), err) | ||
return indexerError(ErrDisconnectBlock, msg) | ||
} | ||
|
||
// Remove the associated spend consumer dependency for the disconnected | ||
// block. | ||
err = idx.Queryer().RemoveSpendConsumerDependency(dbTx, ntfn.Block.Hash(), | ||
idx.consumer.id) | ||
if err != nil { | ||
log.Errorf("%s: unable to remove spend consumer dependency "+ | ||
"for block %s: %v", idx.Name(), ntfn.Block.Hash(), err) | ||
msg := fmt.Sprintf("%s: unable to remove spend consumer "+ | ||
"dependency for block %s: %v", idx.Name(), | ||
ntfn.Block.Hash(), err) | ||
return indexerError(ErrDisconnectBlock, msg) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if intended, but this changes the behavior to return rather than just log errors and the update tip below is no longer always called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's intended, if disconnecting a block fails for the index it'd be inaccurate to update its tip. Also to keep things synchronized, if removing a spend consumer dependency generates an error for an index it should return.
ErrBlockNotOnMainChain = ErrorKind("ErrBlockNotOnMainChain") | ||
) | ||
|
||
// Error satisfies the error interface and prints human-readable errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also check with var _ error = ErrorKind("")
but I guess it will never change with just the one method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferring this for later if we decide it is better to be explicit about implementing the error interface throughout the codebase.
This updates the wire error types to leverage go 1.13 errors.Is/As functionality as well as confirm to the error infrastructure best practices outlined in decred#2181.
f589ae3
to
b6706f5
Compare
This updates the wire error types to leverage go 1.13 errors.Is/As functionality as well as confirm to the error infrastructure best practices outlined in #2181.