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

feat: Add an option to ignore logs about heartbeat message #111

Merged
merged 12 commits into from
Sep 20, 2024

Conversation

broccolism
Copy link
Contributor

@broccolism broccolism commented Aug 13, 2024

Related Issue

Changes

Config
  • Added a new field omit_heartbeat_log on the Config.
    • Default value is false.
RaftNodeCore
  • Check the messageType and the field value to decide whether to log the message.
Examples
  • Changed memstore-static-members and dynamic-members.
    • Added a command line parameter so that we can set the option. Now, we can omit the heartbeat message's log like this: ./misc/bootstrap-static-cluster.tmux.sh 4 --omit-heartbeat-log
etc
  • Added a line on .gitignore for IntelliJ.

Copy link

cla-assistant bot commented Aug 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Aug 13, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@broccolism broccolism changed the title Feature/broccolism/110 Add an option to ignore logs about heartbeat message Aug 13, 2024
@rapsealk
Copy link
Member

Thank you for your contribution!

Just a heads-up: Using one of these keywords will automatically close the original issue when this pull request is merged.

@jopemachine jopemachine changed the title Add an option to ignore logs about heartbeat message feat: Add an option to ignore logs about heartbeat message Aug 17, 2024
raftify/src/config.rs Outdated Show resolved Hide resolved
@jopemachine jopemachine added the feature New feature or request label Sep 12, 2024
Copy link
Contributor Author

@broccolism broccolism left a comment

Choose a reason for hiding this comment

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

@jopemachine Thank you for the review! I've changed as:

  • Remove omit_heartbeat_log field from Config and use config.raft_config. omit_heartbeat_log instead.
  • Change import MessageType from crate::raft::eraftpb, not from jopemachine_raft::eraftpb.
  • Remove omit_heartbeat_log option from memstore examples.

If you want to be able to change the option without recompiling the code, we can refactor the build_config in the example code to read from a separate configuration file.

I agree with you. Maybe I should make another PR to do so..? I guess it would be convenient if we don't need to recompile and restore temporary changes for testing before git commit.


(cc. Since @jollidah wanted to make a new branch from this feature branch, let you know about some changes.)

@jopemachine
Copy link
Member

Thanks!

@jopemachine jopemachine merged commit a610df4 into lablup:main Sep 20, 2024
7 checks passed
@jopemachine
Copy link
Member

jopemachine commented Sep 20, 2024

@broccolism This option can also be added to the Python binding side.

https://github.com/lablup/raftify/blob/main/binding/python/src/bindings/raft_rs/config.rs#L53-L96

@broccolism
Copy link
Contributor Author

@jopemachine I missed this part. I'll make another PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to ignore logs about heartbeat message running examples
3 participants