Add active hosts manager for balance in the future. #304

dangleptr · 2019-04-25T03:24:12Z

subtask of #283

nebula-community-bot · 2019-04-25T03:42:40Z

Unit testing passed.

dutor · 2019-04-26T01:07:03Z

src/meta/ActiveHostsMan.h

+        folly::RWSpinLock::ReadHolder rh(&lock_);
+        hosts.resize(hostsMap_.size());
+        std::transform(hostsMap_.begin(), hostsMap_.end(), hosts.begin(),
+                       [](const auto& entry) -> decltype(auto) {


You know decltype(auto) here will be deduced to a const reference, don't you? It that on purpose? I noticed the value would be copied anyway.

It is OK for const reference due to i want to copy the content.

dutor · 2019-04-26T01:10:48Z

src/meta/ActiveHostsMan.h

+        return !(*this == that);
+    }
+
+    int64_t lastHeartBeatTimeSeconds_ = 0;


This is a time point. So either use lastHeartBeatTimeInSeconds_ or lastHeartBeatTimeSecond_. The current name make me think it's a duration.

How about naming it lastHBTimeInSec?

Make sense.

dutor · 2019-04-26T01:21:38Z

src/meta/test/ActiveHostsManTest.cpp

+    ASSERT_EQ(3, ahMan.getActiveHosts().size());
+    ahMan.updateHostInfo(HostAddr(0, 0), HostInfo(now + 2));
+    ASSERT_EQ(3, ahMan.getActiveHosts().size());
+    ASSERT_EQ(HostInfo(now + 2),  ahMan.hostsMap_[HostAddr(0, 0)]);


There are chances to crash due to race condition.

There are chances to crash due to race condition.
I'm confused, please give me some tips. It is executed in sequence, so why is there a race condition?

dutor · 2019-04-26T01:21:51Z

src/meta/test/ActiveHostsManTest.cpp

+
+    sleep(3);
+    ASSERT_EQ(1, ahMan.getActiveHosts().size());
+    ASSERT_EQ(HostInfo(now + 2),  ahMan.hostsMap_[HostAddr(0, 0)]);


dutor · 2019-04-26T01:26:40Z

src/meta/ActiveHostsMan.h

+namespace nebula {
+namespace meta {
+
+struct HostInfo {


Can we come up with a more specific name? I think this one is a little generic for a exposed name.

Since the data structure only holds the heart beat from the host, how about renaming it to HostHB, HostPulse, or HostHeartbeat?

The structure will be used to collect more information about each host, such as disk usage, cpu idle, memory load etc.

dutor · 2019-04-26T01:34:30Z

src/meta/test/ActiveHostsManTest.cpp

+    ASSERT_EQ(3, ahMan.getActiveHosts().size());
+    ASSERT_EQ(HostInfo(now + 2),  ahMan.hostsMap_[HostAddr(0, 0)]);
+
+    sleep(3);


If I were correct, your interesting host would be expired out in 3 seconds. If so, you better to wait longer than 3s to make the case stable enough, like 3s + 100ms.

The hosts would be expired in 2 seconds.

But the timer might be triggered at the 3rd second.

Timer will be triggered after the first second and run at a 1-second interval.

sherman-the-tank

Thanks for taking care of this. Great job! 👍

I have some inline comments

sherman-the-tank · 2019-04-27T06:17:10Z

src/meta/ActiveHostsMan.h

@@ -0,0 +1,100 @@
+/* Copyright (c) 2018 - present, VE Software Inc. All rights reserved


The company name is vesoft inc, not VE Software Inc. I think we need to go through the copyright declarations in all files and make a batch correction before releasing the code to the public

So let's leave it as it is now :-)

sherman-the-tank · 2019-04-27T06:18:13Z

src/meta/ActiveHostsMan.h

+        return !(*this == that);
+    }
+
+    int64_t lastHeartBeatTimeSeconds_ = 0;


How about naming it lastHBTimeInSec?

sherman-the-tank · 2019-04-27T06:20:32Z

src/meta/ActiveHostsMan.h

+namespace nebula {
+namespace meta {
+
+struct HostInfo {


Since the data structure only holds the heart beat from the host, how about renaming it to HostHB, HostPulse, or HostHeartbeat?

sherman-the-tank · 2019-04-27T06:23:11Z

src/meta/ActiveHostsMan.h

+    explicit HostInfo(int64_t lastHeartBeatTimeSeconds)
+        : lastHeartBeatTimeSeconds_(lastHeartBeatTimeSeconds) {}
+
+    bool operator==(const HostInfo& that) const {


What if this and that refers to different hosts, and their last HB times happen to be the same. Do we consider them same?

Yes. And we should NOT use the structure to check different host Information.

sherman-the-tank · 2019-04-27T06:25:25Z

src/meta/ActiveHostsMan.h

+        CHECK(checkThread_.start());
+        checkThread_.addTimerTask(intervalSeconds * 1000,
+                                  intervalSeconds * 1000,
+                                  &ActiveHostsMan::cleanExpiredHosts, this);


Minor: Put this on the next line to keep one parameter per line

sherman-the-tank · 2019-04-27T06:27:51Z

src/meta/ActiveHostsMan.h

+        checkThread_.wait();
+    }
+
+    void updateHostInfo(HostAddr hostAddr, HostInfo info) {


updateHostHeartbeat(...) probably makes more sense?

HostInfo not only contains heartBeat as talked above.

sherman-the-tank · 2019-04-27T06:31:30Z

src/meta/ActiveHostsMan.h

+        std::vector<HostAddr> hosts;
+        folly::RWSpinLock::ReadHolder rh(&lock_);
+        hosts.resize(hostsMap_.size());
+        std::transform(hostsMap_.begin(), hostsMap_.end(), hosts.begin(),


std::transform() seems an overkill here. Why not just a simple range for

for (const auto& h : hostsMap_) { hosts.emplace_back(h.first); }

Clean and easily understandable!

In addition, we could benefit from the compiler optimization

Actually they are the same thing.

sherman-the-tank · 2019-04-27T06:33:38Z

src/meta/ActiveHostsMan.h

+    void cleanExpiredHosts() {
+        int64_t now = time::TimeUtils::nowInSeconds();
+        folly::RWSpinLock::WriteHolder rh(&lock_);
+        auto it = hostsMap_.begin();


Range for!!

With no need for forcing our developers to use one loop style in our project.

For this case, we have to use iterator explicityly due to the erase operation.

sherman-the-tank · 2019-04-27T06:38:56Z

src/meta/ActiveHostsMan.h

+
+protected:
+    void cleanExpiredHosts() {
+        int64_t now = time::TimeUtils::nowInSeconds();


What you really care about is the time duration, so please use Duration class

In general, unless we have to get the time point (I think it's a very rare case), we should always use Duration, which is monolithic and way faster

I don't think use Duration is more convenient here.

sherman-the-tank · 2019-04-27T06:41:07Z

src/meta/ActiveHostsMan.h

+    std::unordered_map<HostAddr, HostInfo> hostsMap_;
+    thread::GenericWorker checkThread_;
+    int32_t intervalSeconds_ = 0;
+    int32_t expiredSeconds_ = 5 * 60;


I think expirationInSeconds_ is more understandable

nebula-community-bot · 2019-04-28T06:30:57Z

Unit testing failed.

dangleptr · 2019-04-28T06:36:20Z

Jenkins go

nebula-community-bot · 2019-04-28T07:05:14Z

Unit testing passed.

dutor

There you go!

nebula-community-bot · 2019-05-07T06:34:53Z

Unit testing passed.

nebula-community-bot · 2019-05-07T07:32:52Z

Unit testing passed.

* Add active hosts manager for balance in the future. * Address dutor's and sherman's comments

* Fix bashism * Report Gherkin diff * Add PHONY target for tests check-and-diff * Improve clang-format-diff handling Use GITHUB_BASE_REF because a PR can easily have more than one commit: https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables Switch CLANG_HOME handling to support debian/ubuntu installed clang-format or a user's installation at some other location. Install clang-format-10 if it isn't installed in CI (this is mostly for nektos/act where images might be thinner than the GitHub standard). * Get the base commit In order to see what changes this PR is making, we need to get the base commit. * Use actions/checkout to get base and head Co-authored-by: Josh Soref <[email protected]> Co-authored-by: Yee <[email protected]> Co-authored-by: kyle.cao <[email protected]> Co-authored-by: Josh Soref <[email protected]> Co-authored-by: Josh Soref <[email protected]> Co-authored-by: Yee <[email protected]> Co-authored-by: kyle.cao <[email protected]>

dutor reviewed Apr 26, 2019

View reviewed changes

sherman-the-tank reviewed Apr 27, 2019

View reviewed changes

heng added 2 commits April 28, 2019 14:00

Add active hosts manager for balance in the future.

649a828

Address dutor's and sherman's comments

aad12f6

dutor approved these changes May 7, 2019

View reviewed changes

Merge branch 'master' into hb

1c55f2f

Merge branch 'master' into hb

6ef7fef

dangleptr merged commit 3200fd8 into vesoft-inc:master May 7, 2019

dangleptr deleted the hb branch May 7, 2019 07:37

dangleptr mentioned this pull request May 9, 2019

Discover storage service automatically on meta server-side. #283

Closed

tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021

Add active hosts manager for balance in the future. (vesoft-inc#304)

85ed624

* Add active hosts manager for balance in the future. * Address dutor's and sherman's comments

		@@ -0,0 +1,100 @@
		/* Copyright (c) 2018 - present, VE Software Inc. All rights reserved

Add active hosts manager for balance in the future. #304

Add active hosts manager for balance in the future. #304

Conversation

dangleptr commented Apr 25, 2019 • edited Loading

nebula-community-bot commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherman-the-tank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherman-the-tank Apr 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebula-community-bot commented Apr 28, 2019

dangleptr commented Apr 28, 2019

nebula-community-bot commented Apr 28, 2019

dutor left a comment

Choose a reason for hiding this comment

nebula-community-bot commented May 7, 2019

nebula-community-bot commented May 7, 2019

dangleptr commented Apr 25, 2019 •

edited

Loading

sherman-the-tank Apr 27, 2019 •

edited

Loading