Skip to content
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 active hosts manager for balance in the future. #304

Merged
merged 4 commits into from
May 7, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions src/meta/ActiveHostsMan.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/* Copyright (c) 2018 - present, VE Software Inc. All rights reserved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* This source code is licensed under Apache 2.0 License
* (found in the LICENSE.Apache file in the root directory)
*/

#ifndef META_ACTIVEHOSTSMAN_H_
#define META_ACTIVEHOSTSMAN_H_

#include "base/Base.h"
#include <gtest/gtest_prod.h>
#include "thread/GenericWorker.h"
#include "time/TimeUtils.h"

namespace nebula {
namespace meta {

struct HostInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

HostInfo() = default;
explicit HostInfo(int64_t lastHBTimeInSec)
: lastHBTimeInSec_(lastHBTimeInSec) {}

bool operator==(const HostInfo& that) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return this->lastHBTimeInSec_ == that.lastHBTimeInSec_;
}

bool operator!=(const HostInfo& that) const {
return !(*this == that);
}

int64_t lastHBTimeInSec_ = 0;
};

class ActiveHostsMan final {
FRIEND_TEST(ActiveHostsManTest, NormalTest);

public:
ActiveHostsMan(int32_t intervalSeconds, int32_t expiredSeconds)
: intervalSeconds_(intervalSeconds)
, expirationInSeconds_(expiredSeconds) {
CHECK(checkThread_.start());
checkThread_.addTimerTask(intervalSeconds * 1000,
intervalSeconds * 1000,
&ActiveHostsMan::cleanExpiredHosts,
this);
}

~ActiveHostsMan() {
checkThread_.stop();
checkThread_.wait();
}

void updateHostInfo(HostAddr hostAddr, HostInfo info) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateHostHeartbeat(...) probably makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HostInfo not only contains heartBeat as talked above.

folly::RWSpinLock::ReadHolder rh(&lock_);
auto it = hostsMap_.find(hostAddr);
if (it == hostsMap_.end()) {
folly::RWSpinLock::UpgradedHolder uh(&lock_);
hostsMap_.emplace(std::move(hostAddr), std::move(info));
} else {
it->second.lastHBTimeInSec_ = info.lastHBTimeInSec_;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@sherman-the-tank sherman-the-tank Apr 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, we could benefit from the compiler optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually they are the same thing.

[](const auto& entry) -> decltype(auto) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return entry.first;
});
return hosts;
}

protected:
void cleanExpiredHosts() {
int64_t now = time::TimeUtils::nowInSeconds();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

folly::RWSpinLock::WriteHolder rh(&lock_);
auto it = hostsMap_.begin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Range for!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while (it != hostsMap_.end()) {
if ((now - it->second.lastHBTimeInSec_) > expirationInSeconds_) {
LOG(INFO) << it->first << " expired! last hb time "
<< it->second.lastHBTimeInSec_;
it = hostsMap_.erase(it);
} else {
it++;
}
}
}

private:
folly::RWSpinLock lock_;
std::unordered_map<HostAddr, HostInfo> hostsMap_;
thread::GenericWorker checkThread_;
int32_t intervalSeconds_ = 0;
int32_t expirationInSeconds_ = 5 * 60;
};
} // namespace meta
} // namespace nebula

#endif // META_ACTIVEHOSTSMAN_H_
47 changes: 47 additions & 0 deletions src/meta/test/ActiveHostsManTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/* Copyright (c) 2018 - present, VE Software Inc. All rights reserved
*
* This source code is licensed under Apache 2.0 License
* (found in the LICENSE.Apache file in the root directory)
*/
#include "base/Base.h"
#include <gtest/gtest.h>
#include "meta/ActiveHostsMan.h"

namespace nebula {
namespace meta {

TEST(ActiveHostsManTest, NormalTest) {
auto now = time::TimeUtils::nowInSeconds();
ActiveHostsMan ahMan(1, 1);
ahMan.updateHostInfo(HostAddr(0, 0), HostInfo(now));
ahMan.updateHostInfo(HostAddr(0, 1), HostInfo(now));
ahMan.updateHostInfo(HostAddr(0, 2), HostInfo(now));

ASSERT_EQ(3, ahMan.getActiveHosts().size());
ahMan.updateHostInfo(HostAddr(0, 0), HostInfo(now + 2));
ASSERT_EQ(3, ahMan.getActiveHosts().size());
{
folly::RWSpinLock::ReadHolder rh(&ahMan.lock_);
ASSERT_EQ(HostInfo(now + 2), ahMan.hostsMap_[HostAddr(0, 0)]);
}

sleep(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hosts would be expired in 2 seconds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ASSERT_EQ(1, ahMan.getActiveHosts().size());
{
folly::RWSpinLock::ReadHolder rh(&ahMan.lock_);
ASSERT_EQ(HostInfo(now + 2), ahMan.hostsMap_[HostAddr(0, 0)]);
}
}

} // namespace meta
} // namespace nebula


int main(int argc, char** argv) {
testing::InitGoogleTest(&argc, argv);
folly::init(&argc, &argv, true);
google::SetStderrLogging(google::INFO);
return RUN_ALL_TESTS();
}


16 changes: 16 additions & 0 deletions src/meta/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,19 @@ nebula_link_libraries(

nebula_add_test(meta_client_test)

add_executable(
active_hosts_man_test
ActiveHostsManTest.cpp
$<TARGET_OBJECTS:base_obj>
$<TARGET_OBJECTS:thread_obj>
$<TARGET_OBJECTS:time_obj>
$<TARGET_OBJECTS:fs_obj>
$<TARGET_OBJECTS:network_obj>
)
nebula_link_libraries(
active_hosts_man_test
gtest
)

nebula_add_test(active_hosts_man_test)