Improved xcvrd shutdown flow #1
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keboliu
reviewed
Feb 18, 2019
sonic-xcvrd/scripts/xcvrd
Outdated
| @@ -90,19 +93,17 @@ def log_error(msg, also_print_to_console=False): | |||
| #========================== Signal Handling ========================== | |||
|
|
|||
| def signal_handler(sig, frame): | |||
| global RUN | |||
There was a problem hiding this comment.
Kevin has submitted a PR to unify helper functions that used by all the daemons, signal handler is part of it since his PR is approved and above to merge, I think you need to look at his PR and align with him. Here is his PR: sonic-net/sonic-buildimage#2570
There was a problem hiding this comment.
@keboliu i have reviewed Kevin's changes and i am strongly disagree with a such approach. I had a discussion with @andriymoroz-mlnx and decided to open this PR as is.
My major concerns are:
- Incorrect usage of signal handlers: throwing an exception in application main process thru calling sys.exit inside a signal handler is a bad practise
- Placing a signal handlers in common class is also a bad practise
I think that each daemon should have a graceful shutdown flow.
nazariig
force-pushed
the
master-improve-xcvrd-shutdown
branch
3 times, most recently
from
b7461e3
to
9ec1636
Compare
nazariig
force-pushed
the
master-improve-xcvrd-shutdown
branch
from
fd1e078
to
aee8250
Compare
Signed-off-by: Nazarii Hnydyn <[email protected]>
nazariig
force-pushed
the
master-improve-xcvrd-shutdown
branch
from
aee8250
to
13c1f22
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Nazarii Hnydyn [email protected]