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.
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
chore(#9585): teach Sentinel sign language #9658
chore(#9585): teach Sentinel sign language #9658
Changes from 4 commits
b3ec5d6
f7f715b
b8042b4
ea2b903
8179937
e6b4344
634b863
da85e61
9c6dd01
284fe01
547a96a
f0095d6
8ce84a6
6dacf1e
145b258
4b575de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I struggled to find any documentation on this but I think we can call the signals whatever we want, right? If so let's use clearer names. What about
SIGLISTEN
,SIGSTOPLISTENING
, andSIGRUNTASKS
? Or evenSIGSENTINELLISTEN
etc? It's getting long but at least it'll be clear that it's a custom signal and not a Node thing?Also I think
SIGUSR1
is reserved and according to this documentation will trigger Node into debugging mode: https://nodejs.org/en/learn/getting-started/debugging - so if we can't rename as above, let's use a different number at least.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.
Unfortunately, you can't send custom signals:
So you need to select one of the signals in the list. There are some random ones that might not kill the process, but most do, and it felt weird to send a
SIGURG
to stop feed listening.This is why there's a toggle to stop/start processing, because we can't send custom signals and there are only two which are "user customizable" - SIGUSR1 and SIGUSR2, which I am using.
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.
Ugh that's disgusting. Ok. The only other way I can see to do it would be to have a single signal which then looks somewhere else (file or db) to reload config, but that almost defeats the purpose!
Let's go with what you have.