-
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
Use active hosts when creating space #425
Conversation
Jenkins go |
Unit testing passed. |
if ((int32_t)hosts.size() < replicaFactor) { | ||
LOG(ERROR) << "Not enough hosts existed for replica " | ||
<< replicaFactor << ", hosts num " << hosts.size(); | ||
resp_.set_code(cpp2::ErrorCode::E_UNSUPPORTED); |
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.
Unsupport
is the meaning that this operation does not implement.
Illegal_Argument
will be better ?
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.
Thrift will return its own failure if related method does not implement. So i think UNSUPPORTED is ok in this case.
if (FLAGS_hosts_whitelist_enabled | ||
&& 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.
LOG(ERROR)?
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.
Actually, it is not an error.
Let's merge it firstly to avoid pending tasks in the next. |
Jenkins go |
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.
Do we need to call registerHB in all ut with listHosts?
Unit testing passed. |
1 similar comment
Unit testing passed. |
This SHOULD NOT be the reason to get PR approved! PLEASE remember you are responsible for your vote! We need to make sure all code committed are high quality, not temporary. Especially NOT just to avoid merge conflict. |
Do you have any concerns about this pr? @sherman-the-tank |
This pr have been reviewed, don't you see the comments by @darionyaphet ? You could have no suggestions or comments, and you could vote -1 if you have any other thoughts, but please don't say the pr is NOT reviewed just because you haven't reviewed it. @sherman-the-tank |
* Use active hosts when creating space * x
* Use active hosts when creating space * x
…dges if we use go reversely (vesoft-inc#425) * Fix the issue that the query results will still contain the expired edges if we use go reversely * fix the code lint error Co-authored-by: Doodle <[email protected]> Co-authored-by: Ryan <[email protected]> Co-authored-by: Doodle <[email protected]>
subtask of #283
Add a flag hosts_whitelist_enabled to control whether we filter host in white list or not after received hb.