-
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
Support displaying online hosts in "SHOW HOSTS" command and save hosts info in rocskdb #450
Conversation
You could run UTs locally with ctest, and the failure could be check on http://ci.vesoft.com/job/Nebula-Graph-UnitTest/ |
Close #440 |
jenkins go |
Unit testing passed. |
Please rebased on master |
std::vector<uniform_tuple_t<std::string, 2>> expected{ | ||
{"127.0.0.1", "1000"}, | ||
{"127.0.0.1", "1100"}, | ||
std::vector<uniform_tuple_t<std::string, 3>> expected{ |
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'd better test one case with host offline.
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.
roger that
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.
SchemaTest has other ut which need to use active hosts. I add ut in meta/test/ProcessorTest.
const auto& prefix = MetaServiceUtils::hostPrefix(); | ||
std::unique_ptr<kvstore::KVIterator> iter; | ||
auto ret = kvstore_->prefix(kDefaultSpaceId_, kDefaultPartId_, prefix, &iter); | ||
if (ret != kvstore::ResultCode::SUCCEEDED) { |
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.
If whitelist disabled, no hosts existed in rocksdb is normal.
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.
nice catch
jenkins go |
Unit testing passed. |
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.
Nice and clean code.
jenkins go |
Unit testing passed. |
1 similar comment
Unit testing passed. |
src/interface/meta.thrift
Outdated
@@ -89,6 +89,11 @@ struct EdgeItem { | |||
4: common.Schema schema, | |||
} | |||
|
|||
struct HostItem { | |||
1: common.HostAddr hostAddr, | |||
2: string status, |
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'd better use one enum class to represent status.
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
src/meta/processors/BaseProcessor.h
Outdated
/** | ||
* Get all hosts with online/offline status. | ||
* */ | ||
StatusOr<std::vector<cpp2::HostItem>> allHostsWithStatus(); |
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'd better put the method in ListHostProcessor if no other processors use it.
Unit testing failed. |
jenkins go |
Jenkins go |
Unit testing failed. |
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.
The pr looks better now.
src/meta/processors/BaseProcessor.h
Outdated
@@ -23,6 +23,9 @@ namespace meta { | |||
|
|||
using nebula::network::NetworkUtils; | |||
|
|||
const PartitionID kDefaultPartId_ = 0; |
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.
Put it some where more common, use "static const" and remove the underscore in name.
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.
Some processors only depends on this header file, I will rename it anyway.
Unit testing failed. |
1 similar comment
Unit testing failed. |
Jenkins go |
Unit testing failed. |
Unit testing passed. |
Unit testing failed. |
1 similar comment
Unit testing failed. |
Unit testing passed. |
1 similar comment
Unit testing passed. |
Unit testing passed. |
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.
Well done. LGTM now
Unit testing passed. |
Unit testing passed. |
…s info in rocskdb (vesoft-inc#450) * Support displaying online hosts in "SHOW HOSTS" command [440] * remove ActivHostsManHolder, store host info in rocksdb * Use call_once to init ActiveHostsMan. Rebased on master.
…s info in rocskdb (vesoft-inc#450) * Support displaying online hosts in "SHOW HOSTS" command [440] * remove ActivHostsManHolder, store host info in rocksdb * Use call_once to init ActiveHostsMan. Rebased on master.
as title #440