-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Implement tracing & thiserror
error types
#29
Conversation
@zeeshanlakhani Tagged you as a reviewer on this PR, specifically for the tracing commit ( 436c1b6 ), since you're more knowledgeable on tracing. LMK what you think ✌️ |
Clippy warning is triggered within the A little annoying. I'll try ignoring the warning until that issue is closed. |
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.
LGTM 🎉!
} | ||
|
||
match dag_verification.block_state(cid) { | ||
BlockState::Have => continue, | ||
BlockState::Unexpected => { | ||
eprintln!("Warn: Received block {cid} out of order, may be due to bloom false positive."); | ||
trace!( |
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.
trace! is the lowest level. Should this be a warn?
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.
It's trace intentionally.
I used to think of this as a warning - it's one of the things that if something goes wrong, this may happen more often.
But it's also something that just happens during normal execution, so more like some info.
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.
ah ok. yeah, from the comment it made it sound worse than it is :).
This implements tracing messages using the
tracing
crate and error types using thethiserror
crate.Closes #25
Closes #26
I'm somewhat waiting for n0-computer/iroh-car#12 to land so I can improve the error types a bit.
Additionally, it'd be nice to have a better
BlockStore
trait eventually, so (1) we can get rid off theBlockStoreIncompatible
error and (2) use a custom error type from theBlockStore
instead of a hard-codedanyhow::Error
.