-
Notifications
You must be signed in to change notification settings - Fork 50
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/better exit messages #657
Conversation
@nicolasochem does that solve your issue? |
@rvermootenct work effort missing. I propose to merge today and make further improvements in a separate PR |
…r-organization/tezos-reward-distributor into feat/better_exit
@rvermootenct @jdsika thanks! It does sound useful to have a utility, and I think having 3 possible exit codes is fine. After a cursory look, it looks like you are only catching USER_ABORT here? And never GENERAL_ERROR? Can you also add the case where the program ends in error:
These are the scenarios for which I would like my infra to alert me. |
@rvermootenct I would like to include this in a release that I want to start to prepare. I think we need to put a date on this one |
When will you finish this PR? We will soon have a new protocol version |
It looks like the last commit adds multiple error types, but it still does not catch the generic error as I was calling for, is this correct @rvermootenct ? |
make sure to integrate #611 here "clear lockfile when properly shut down" "Properly" means in this case that there are no threads running in the background anymore which could potentially trigger a payment. |
This PR only gives better exit messages and exit codes. The state machine in this thing is very confusing to me and I've tried to make sense of it but I think someone else will be better equipped to figure out how to ensure the lockfiles are correctly removed at the correct times. As I'm moving away from this project/ecosystem I don't think it's worth anyones while me spending time struggling through this. I'm willing to spend some time today to try figure this out, but if I can't crack it I'd like to not add to this PR. I'd like this work to be thoroughly checked too because I don't want this pr to incorrectly exit program. @nicolasochem I hope now I have caught the generic errors. If not can we please have a chat sometime this week and you can explain to me the situation? |
src/cli/client_manager.py
Outdated
raise ClientException( | ||
"Unknown Error at signing. Please consult the verbose logs!" | ||
exit_program( | ||
ExitCode.SIGNER_ERROR_NOT_RUNNING, ExitMessage.SIGNER_ERROR_NOT_RUNNING |
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.
This can be any other error including "the signer not running", correct?
src/pay/payment_consumer.py
Outdated
if disk_is_full(): | ||
running = False | ||
break | ||
exit_program(ExitCode.NO_SPACE) |
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.
Why remove the break here and exit directly? Doesn't this change the behavior?
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.
well it would just leave the run at the moment and break, but if the disk is full why not just exit?
I tried the code while my signer was not running and got nested exceptions.
|
It's still not working. When signer is off, I'm still getting
An error message is needed here: https://github.com/tezos-reward-distributor-organization/tezos-reward-distributor/pull/657/files#diff-acda51ab96991dc20d4760e24db3206e89b406b0ee525a93ef6ff7ed290dbd44R386 |
Another error I got:
|
@vkresch I see also a topic with the logger. I think the global logger should trigger the exit of the function when an error log is thrown right? |
@jdsika please post pone this feature as I would like to have more time to look into it. Currently the implementation seems not to fix the initial issue. |
I would call it "temporarily disabled until time for a fix" but if you want to call the "removal of the feature" :D |
IMO and error in the log must trigger a graceful shutdown - yes! |
Work effort: 4h |
@nicolasochem @jdsika could you test your usecase again? |
@vkresch it works now, I tried 3 things:
So it looks like you fixed the issue 👍 |
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 works after quick testing. Failure modes result in non-zero exit status.
@nicolasochem gonna try to fix the tests today here and then we can merge |
@jdsika @nicolasochem mergable if needed |
name: Pull Request
about: Create a pull request to make a contribution
labels:
IMPORTANT NOTICE:
I read and understood the guidelines for contributions to the TRD. The contribution may qualify for being compensated by the TRD grant if approved by the maintainers.
This PR resolves the issue #653 . The following steps were performed:
Analysis: It would be nice for TRD to give exit messages that arn't just 0. It would also be nice to have a utility we could add to with different exit situations.
Solution: Create that utility
Implementation: Created a lil exit utility and attached it to the log file duplicate check.
Performed tests: Added tests and ran it with a lockfile present, confirmed exit code was not 0.
Documentation: I think the enum idea self documents? I'm on the fence about having different exit codes for different exit reasons we can come up with or just have 0 and 1, thoughts @nicolasochem?
Work effort: