-
Notifications
You must be signed in to change notification settings - Fork 76
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
refactor(composer): streamline geth and executor run_until_stopped
#1361
refactor(composer): streamline geth and executor run_until_stopped
#1361
Conversation
….com/astriaorg/astria into ENG-670/add_composer_instrumentation
…://github.com/astriaorg/astria into ENG-670/refactor_for_composer_instrumentation
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.
A few nitpicks, but the moved logic all seems to be unchanged.
Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>
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 mosly fine except for some oddities.
Requesting changes because an early return in the select loop skips shutdown logic.
} else { | ||
// log all the bundles that have not been drained | ||
let report: Vec<SizedBundleReport> = | ||
bundles_to_drain.iter().map(SizedBundleReport).collect(); |
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.
I think we might be able to avoid the extra allocation here entirely by using: https://docs.rs/serde/latest/serde/ser/trait.Serializer.html#method.collect_seq. But leave it to a followup probably.
Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
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 great, thank you for your work. Made a suggestion to clean up report_exit_reason
, but otherwise this is good to merge.
c876d2b
into
ENG-670/add_composer_instrumentation
Summary
Shortened and streamlined geth and executor
run_until_stopped()
functions to get rid of clippy exceptions and ensure events are emitted within spans.Background
#1326 removed instrumentation on these
run_until_stopped()
functions, which revealed a clippy warning for too many lines. Additionally, this made it so that logging inside these functions would not be emitted within any span.Changes
utils
module to house a sharedreport_exit_reason()
function.ensure_chain_id_is_correct()
andget_latest_nonce()
toinit()
(previosulypre_run_checks()
)Testing
Passing all tests.
Related Issues
Part of #1321