-
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
Adjust meta client ctor && implement heartbeat logic inside. #383
Conversation
Unit testing passed. |
1 similar comment
Unit testing passed. |
src/daemons/StorageDaemon.cpp
Outdated
<< ", status:" << result.status(); | ||
return EXIT_FAILURE; | ||
} | ||
uint32_t localIP; |
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.
Add a log to print localIP?
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.
Good point.
src/daemons/StorageDaemon.cpp
Outdated
|
||
if (FLAGS_meta_server_addrs.empty()) { |
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 should add a regular to check format of FLAGS_meta_server_addrs?
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.
Good proposal. We could check the format of server_address inside NetWorkUtils::toHosts
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.
FYI. gflags supports some kind of validator that will be checked against whenever a specific flag is being modified. We could utilize that at the first place for most of our flags. We could do that in future.
src/graph/ExecutionEngine.cpp
Outdated
@@ -22,7 +24,11 @@ ExecutionEngine::~ExecutionEngine() { | |||
|
|||
|
|||
Status ExecutionEngine::init(std::shared_ptr<folly::IOThreadPoolExecutor> ioExecutor) { | |||
metaClient_ = std::make_unique<meta::MetaClient>(); | |||
if (FLAGS_meta_server_addrs.empty()) { |
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.
ditto
Unit testing passed. |
|
||
auto metaClient = std::make_unique<nebula::meta::MetaClient>(); | ||
auto ioThreadPool = std::make_shared<folly::IOThreadPoolExecutor>(FLAGS_io_handlers); |
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 could share the same thread pool with the one in ThriftServer
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 have done it. Please note line 151
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.
Excellent.
@@ -24,7 +26,7 @@ using nebula::cpp2::ValueType; | |||
using apache::thrift::FragileConstructor::FRAGILE; | |||
|
|||
TEST(MetaClientTest, InterfacesTest) { | |||
FLAGS_load_data_interval_second = 1; | |||
FLAGS_load_data_interval_secs = 1; |
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.
As for these tests, I think there are two ways to make them faster. First, we could have shared the same MockServer instead of launch one per test. Second, seconds is appropriate for a polling interval, but too long for unit testing, while I cannot come up with a better way. But you still could impove it by usleep(FLAGS_load_data_interval_secs * 1000000 + 100000/*100ms*/)
to save 0.9s per sleep.
Of course, we could leave these work to the next PR.
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
Jenkins go |
Unit testing passed. |
Jenkins go |
Unit testing failed. |
Damn it! Conflicts again. |
Jenkins go |
Unit testing failed. |
1 similar comment
Unit testing failed. |
Jenkins go |
Unit testing failed. |
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.
LGTM
…inc#383) * Adjust meta client ctor && implement heartbeat logic inside. * Address laura-ding's comments * Fix conflicts
…inc#383) * Adjust meta client ctor && implement heartbeat logic inside. * Address laura-ding's comments * Fix conflicts
* add cachelib and storage cache class include new files change cmakelist update thrift fix build fix build fix build add constructor for struct clean and add test add cmake related fix test trying cmake add more test and cmake change cmake trying out failed and revert fix test bug now fix cmake add a test fix poolname bug delete a test add lock update conf * update format and change name * fix typo in cmake * remove unneeded lib in cmake * fix cmake * fix comment change to use & in put * add some log and update signature of functions in storage cache * add function getting cache hit count * update config * update config * change the vertex pool ttl default value * format * fix typo * change to use error code * fix typo * typo * update error code value * add lrt Co-authored-by: Doodle <[email protected]>
Subtask of #283
After the repo to be public, changes could NOT be rebased on the original pr #379
So Let's use the new one to track it.