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

IF: Add logger for vote related logging #28

Merged
merged 9 commits into from
Apr 19, 2024
Merged

Conversation

heifner
Copy link
Member

@heifner heifner commented Apr 15, 2024

  • Replaces hotstuff logger with vote logger.
  • Move all vote related logging to vote logger.
  • Optimize default logger
    • Reduced mutex locks by 1
    • Removed string copy of DEFAULT_LOGGER name on every call
    • Removed 1 shared_ptr creation per call

Resolves #14

@heifner heifner requested review from linh2931 and greg7mdp April 15, 2024 13:14
@heifner heifner added the OCI Work exclusive to OCI team label Apr 15, 2024
@heifner heifner linked an issue Apr 15, 2024 that may be closed by this pull request
// Called from net threads
vote_status block_state::aggregate_vote(const vote_message& vote) {
// Called from vote threads
vote_status block_state::aggregate_vote(uint32_t connection_id, const vote_message& vote) {
Copy link
Member

Choose a reason for hiding this comment

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

connection_id has nothing to do with voting, it is here only for logging. Not sure if it is appropriate to add it as a parameter to vote related functions. No other ways to log connection_id?

Copy link
Member

Choose a reason for hiding this comment

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

Or hide it inside vote_message?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't put it in vote_message. It is really nice to have it for debugging.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Just add a comment in the function interface indicating connection_id is only for debugging.

// Called from net threads
vote_status block_state::aggregate_vote(const vote_message& vote) {
// Called from vote threads
vote_status block_state::aggregate_vote(uint32_t connection_id, const vote_message& vote) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be slightly better to pass std::optional<uint32_t> so that the API would work even if the add_vote didn't come from a net connection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it uses 0 for that, but could go with optional if you think it is cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally up to you.

@ericpassmore
Copy link
Contributor

Note:start
group: IF
category: INTERNALS
summary: Move vote logging messages from net_plugin_impl logger to new vote logger. Done to move load off net_plugin_impl logger.
Note:end

Base automatically changed from GH-3-process-votes to savanna April 19, 2024 11:20
@heifner heifner merged commit 395724e into savanna Apr 19, 2024
36 checks passed
@heifner heifner deleted the GH-14-vote-loggin branch April 19, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move net_plugin vote message logging to different logger
4 participants