-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add a version check to clouseau connected() function #5417
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
Conversation
mckenzr
left a comment
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.
LGTM
mckenzr
left a comment
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.
LGTM
jiahuili430
left a comment
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.
+1
cf546d2 to
d576d83
Compare
A ping is not enough. A clouseau instance is not usable if we it's just connected, it's main process should be up and running as well. We have observed that it is possible for the clouseau node to be connected and respond to pings, but the main process was down. We want to alert or catch cases when that happens so we ask the main process for the version.
d576d83 to
f09da22
Compare
jaydoane
left a comment
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.
Tested against Clouseau 2.24
(node1@127.0.0.1)6> clouseau_rpc:connected().
true
| % {'EXIT',noconnection} | ||
| % | ||
| case (catch version()) of | ||
| {ok, _} -> true; |
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.
Haha was just trying to suggest that change and you beat me to it!
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.
You did suggest it in the meeting, so thanks for your input, Jay!
A ping is not enough. A clouseau instance is not usable if we it's just connected, it's main process should be up and running as well.
We have observed that it is possible for the clouseau node to be connected and respond to pings, but the main process was down. We want to alert or catch cases when that happens so we ask the main process for the version.