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

Add an error when a node fails when using dora run #719

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

haixuanTao
Copy link
Collaborator

Currently, when using dora run a node failure does not raise any warning nor error.

This makes errors silent.

This PR makes the error explicit.

Copy link
Collaborator

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

I think the proper place to log these errors is the send_log_message method in binaries/daemon/src/lib.rs. We normally send the log messages to the coordinator if connected. We could add an else branch there that logs the messages with their specified level.

@haixuanTao haixuanTao force-pushed the add-error-on-node-failure branch from f33da2f to dff8f74 Compare November 25, 2024 05:18
@haixuanTao
Copy link
Collaborator Author

I think the proper place to log these errors is the send_log_message method in binaries/daemon/src/lib.rs. We normally send the log messages to the coordinator if connected. We could add an else branch there that logs the messages with their specified level.

Done!

I made some small tweak to avoid redundancy.

@haixuanTao haixuanTao force-pushed the add-error-on-node-failure branch from dff8f74 to 5f76ea3 Compare November 27, 2024 13:24
@haixuanTao haixuanTao enabled auto-merge November 27, 2024 13:24
@haixuanTao haixuanTao disabled auto-merge November 27, 2024 14:09
@haixuanTao haixuanTao merged commit 206be36 into main Nov 27, 2024
102 checks passed
@haixuanTao haixuanTao deleted the add-error-on-node-failure branch November 27, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants