-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 heartbeat processor #343
Conversation
Unit testing passed. |
"Check the expired hosts at the interval"); | ||
DEFINE_int32(expired_threshold_sec, 10 * 60, | ||
"Hosts will be expired in this time if no heartbeat received"); | ||
|
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.
Would a parameter check be better? for example :
static bool checkHBFlags(const char* fn, int32_t th) {
if (FLAGS_expired_hosts_check_interval_sec > th)
return false;
return true;
}
static const bool invalid = RegisterFlagValidator(&FLAGS_expired_threshold_sec, &checkHBFlags);
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.
Sounds good.
auto hostKey = MetaServiceUtils::hostKey(h.ip, h.port); | ||
auto ret = hostExist(hostKey); | ||
if (!ret.ok()) { | ||
LOG(WARNING) << "The host [" << h.ip << ":" << h.port << "] not existed!"; |
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.
Here ip is int32, not dotted decimal IP string, so even if you put it into the log, the user can't understand.
so I think we should put dotted decimal IP string to log.
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.
Make sense. I noticed output HostAddr to stream has been supported.
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.
Yeah, output HostAddr to stream has been supported. 👍
so here we should output h
.
HostAddr host(req.host.ip, req.host.port); | ||
if (hostExist(MetaServiceUtils::hostKey(host.first, host.second)) | ||
== Status::HostNotFound()) { | ||
LOG(INFO) << "Reject unregistered host " << host << "!"; |
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 think we should add a interface to handler the situation uniformly.
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.
Output HostAddr to stream has been supported.
} | ||
|
||
struct HBReq { | ||
1: common.HostAddr host, |
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.
timestamp ?
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.
For what?
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.
when a storage node offline, could get the final reporting time.
Or recorded it in logs
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.
We have recorded its last hb-timestamp
Unit testing failed. |
Unit testing passed. |
LGTM, Great Work 🎉 |
Jenkins go |
Unit testing passed. |
Unit testing passed. |
* x * Add heartbeat processor * Address comments
* “enhancement-template” * “fix-feature-request” * Update feature_request.md Co-authored-by: Shylock Hg <[email protected]> Co-authored-by: Sophie-Xie <[email protected]> Co-authored-by: Shylock Hg <[email protected]>
Subtask of #283