-
Notifications
You must be signed in to change notification settings - Fork 550
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
log file for rejected blocks #7605
Conversation
Will the new file also show up in coda client export-logs and -export-local-logs ? |
Yup, any file with substring |
~processor:(Logger.Processor.raw ()) | ||
~transport: | ||
(Logger.Transport.File_system.dumb_logrotate ~directory:conf_dir | ||
~log_filename:"mina-best-tip.log" ~max_size:best_tip_diff_log_size | ||
~num_rotate:1) ; | ||
let rejected_blocks_log_size = 1024 * 1024 * 5 in |
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 might be more elegant (and easier to use again in future etc.) to add the concept of Logger_id.t
extensions to Logger.Consumer_registry
, so that this logger could accept messages and write them to its file but also pass them to the standard logger automatically too.
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.
Agreed, is it ok if I follow it up in another PR?
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.
Good idea, yea
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.
Changes look alright, but I don't love the duplicated logging logic.
A separate log file
mina-rejected-blocks.log
for rejected blocks. This is to understand the impact of blocks being missed/not broadcasted fast enough(tracked here #7338)