Implement admin client for balancer #553

dangleptr · 2019-06-28T07:36:07Z

The PR is based on #516 .

Subtask of #515

src/meta/processors/admin/BalancePlan.cpp

src/meta/processors/admin/BalancePlan.h

src/meta/processors/admin/Balancer.cpp

dangleptr · 2019-07-04T11:08:55Z

Jenkins go

nebula-community-bot · 2019-07-04T11:46:44Z

Unit testing passed.

whitewum · 2019-07-12T03:07:59Z

src/meta/test/BalanceIntegrationTest.cpp

+namespace nebula {
+namespace meta {
+
+TEST(BalanceIntegrationTest, SimpleTest) {


more tests on going?

Yes. The BalanceIntegrationTest will go on when server-side is ready.

whitewum · 2019-07-12T03:53:53Z

src/meta/processors/admin/AdminClient.cpp

+            case storage::cpp2::ErrorCode::E_LEADER_CHANGED: {
+                if (retry < retryLimit) {
+                    HostAddr leader(resp.get_leader().get_ip(), resp.get_leader().get_port());
+                    int32_t leaderIndex = 0;


clear to 0 every time?

The code here is to find the index of leader in hosts.

whitewum · 2019-07-12T03:55:15Z

src/meta/processors/admin/AdminClient.cpp

+    });
+}
+
+nebula::cpp2::HostAddr AdminClient::to(const HostAddr& addr) {


better naming than to()

How about toThriftHost ?

sounds better

whitewum · 2019-07-12T03:55:51Z

src/meta/processors/admin/AdminClient.cpp

+        }
+        case kvstore::ResultCode::ERR_KEY_NOT_FOUND:
+            return Status::Error("Key Not Found");
+        default:


need a log?

Good point.

whitewum · 2019-07-12T03:56:19Z

src/meta/processors/admin/AdminClient.cpp

+
+StatusOr<std::vector<HostAddr>> AdminClient::getPeers(GraphSpaceID spaceId, PartitionID partId) {
+    CHECK_NOTNULL(kv_);
+    auto partKey = MetaServiceUtils::partKey(spaceId, partId);


Compiler will optimize it. The partKey will reuse the returned string's allocation.

whitewum · 2019-07-12T03:59:01Z

src/meta/processors/admin/BalanceTask.cpp

@@ -38,7 +38,7 @@ void BalanceTask::invoke() {
        case Status::CHANGE_LEADER: {
            LOG(INFO) << taskIdStr_ << "Ask the src to give up the leadership.";
            SAVE_STATE();
-            client_->transLeader(spaceId_, partId_, src_, dst_).thenValue([this](auto&& resp) {
+            client_->transLeader(spaceId_, partId_, src_).thenValue([this](auto&& resp) {


line 36: ms or s ?
BTW, do more logs in this function: a lot of cases for state change tracking

Good catch. We should use MS here.

whitewum · 2019-07-12T04:18:02Z

src/meta/test/AdminClientTest.cpp

+        ASSERT_EQ(HostAddr(localIp, sc2->port_), hosts[0]);
+        ASSERT_EQ(HostAddr(localIp, sc1->port_), hosts[1]);
+        ASSERT_EQ(HostAddr(1, 1), hosts[2]);
+    }


let's try more than 3 hosts.

Yes. We have more situations to test. Let's open another issue to track it.

whitewum · 2019-07-12T04:20:45Z

src/meta/test/AdminClientTest.cpp

+            baton.post();
+        });
+        baton.wait();
+    }


what about add part again?
or let's loop the test a few times

AddPart again will be tested on server side. The UT is just to test client interface.

Yep. It's simple for the client, but a valued corned case.

nebula-community-bot · 2019-07-12T10:09:07Z

Unit testing failed.

dangleptr · 2019-07-15T02:10:33Z

Jenkins go

nebula-community-bot · 2019-07-15T02:32:02Z

Unit testing passed.

wadeliuyi · 2019-07-15T02:42:52Z

src/meta/processors/admin/AdminClient.cpp

+    req.set_space_id(spaceId);
+    req.set_part_id(partId);
+    req.set_as_learner(asLearner);
+    return getResponse(host, std::move(req), [] (auto client, auto request) {


I can not understand why need move(req) here, the req’s memory is in caller stack, and the parameter req of getResponse passed by value?

Maybe req will be changed in the future. So use move here.

wadeliuyi · 2019-07-15T02:44:02Z

src/meta/processors/admin/AdminClient.cpp

+    }
+    folly::Promise<Status> pro;
+    auto f = pro.getFuture();
+    getResponse(ret.value(), 0, req, [] (auto client, auto request) {


and here why not move(req)?

A HA. Good catch! We should use move.

nebula-community-bot · 2019-07-15T04:55:00Z

Unit testing passed.

wadeliuyi

LGTM

nebula-community-bot · 2019-07-15T08:35:08Z

Unit testing failed.

whitewum

Please consider some corner cases: 1.operations failed at the server side, or 2. client crashed and resume to do some job (what should client do).

whitewum · 2019-07-15T08:46:03Z

src/meta/processors/admin/AdminClient.cpp

+                        }
+                        leaderIndex++;
+                    }
+                    LOG(INFO) << "Return leder change from " << hosts[index]


whitewum · 2019-07-15T08:46:51Z

src/meta/processors/admin/AdminClient.cpp

+    });
+}
+
+nebula::cpp2::HostAddr AdminClient::to(const HostAddr& addr) {


sounds better

whitewum · 2019-07-15T08:50:26Z

src/meta/test/AdminClientTest.cpp

+    }
+}
+
+TEST(AdminClientTest, RetryTest) {


whitewum · 2019-07-15T08:52:43Z

src/meta/test/AdminClientTest.cpp

+            baton.post();
+        });
+        baton.wait();
+    }


Yep. It's simple for the client, but a valued corned case.

nebula-community-bot · 2019-07-15T09:39:03Z

Unit testing passed.

dangleptr · 2019-07-15T10:40:47Z

Please consider some corner cases: 1.operations failed at the server side, or 2. client crashed and resume to do some job (what should client do).

Yes, all interfaces here reentrant functions. When we implement them on server-side, we should keep the principle

wadeliuyi

LGTM

nebula-community-bot · 2019-07-15T10:52:40Z

Unit testing passed.

* Implement AdminClient * Address whitewum's comments * Address wadeliuyi's comments * Fix failed compile

dangleptr added the do not review PR: not ready for the code review yet label Jun 28, 2019

dangleptr added this to the v1_beta_release milestone Jun 28, 2019

dangleptr mentioned this pull request Jun 28, 2019

Balance skeleton on meta. #516

Merged

wadeliuyi reviewed Jun 28, 2019

View reviewed changes

src/meta/processors/admin/BalancePlan.cpp Outdated Show resolved Hide resolved

wadeliuyi reviewed Jun 28, 2019

View reviewed changes

src/meta/processors/admin/BalancePlan.cpp Outdated Show resolved Hide resolved

wadeliuyi reviewed Jun 28, 2019

View reviewed changes

src/meta/processors/admin/BalancePlan.h Outdated Show resolved Hide resolved

wadeliuyi reviewed Jun 28, 2019

View reviewed changes

src/meta/processors/admin/Balancer.cpp Outdated Show resolved Hide resolved

wadeliuyi reviewed Jul 1, 2019

View reviewed changes

src/meta/processors/admin/Balancer.cpp Outdated Show resolved Hide resolved

dangleptr force-pushed the balancer-client branch from 0b45406 to eb911d0 Compare July 4, 2019 11:00

dangleptr requested review from critical27 and whitewum July 4, 2019 11:08

dangleptr added ready-for-testing PR: ready for the CI test and removed do not review PR: not ready for the code review yet labels Jul 4, 2019

dangleptr requested review from sherman-the-tank, a user, CPWstatic, darionyaphet, dutor, laura-ding and wadeliuyi July 4, 2019 11:09

whitewum reviewed Jul 12, 2019

View reviewed changes

dangleptr force-pushed the balancer-client branch from eb911d0 to f92944b Compare July 12, 2019 09:56

wadeliuyi reviewed Jul 15, 2019

View reviewed changes

dangleptr force-pushed the balancer-client branch from 79d84d6 to f00dc93 Compare July 15, 2019 04:33

wadeliuyi previously approved these changes Jul 15, 2019

View reviewed changes

heng added 3 commits July 15, 2019 16:40

Implement AdminClient

baa29c3

Address whitewum's comments

26d4a87

Address wadeliuyi's comments

8c9a4ac

whitewum previously approved these changes Jul 15, 2019

View reviewed changes

Fix failed compile

4684f43

dangleptr dismissed stale reviews from whitewum and wadeliuyi via 4684f43 July 15, 2019 09:10

dangleptr force-pushed the balancer-client branch from 41e4253 to 4684f43 Compare July 15, 2019 09:10

Merge branch 'master' into balancer-client

e97cf78

whitewum approved these changes Jul 15, 2019

View reviewed changes

wadeliuyi approved these changes Jul 15, 2019

View reviewed changes

dangleptr merged commit 0a3fbc4 into vesoft-inc:master Jul 15, 2019

dangleptr deleted the balancer-client branch July 15, 2019 10:52

yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Feb 16, 2020

Implement admin client for balancer (vesoft-inc#553)

cad6d6f

* Implement AdminClient * Address whitewum's comments * Address wadeliuyi's comments * Fix failed compile

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

Implement admin client for balancer (vesoft-inc#553)

f37297b

* Implement AdminClient * Address whitewum's comments * Address wadeliuyi's comments * Fix failed compile

yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022

fix compilation after disabling v1 upgrader (vesoft-inc#553)

01377c9

Implement admin client for balancer #553

Implement admin client for balancer #553

Conversation

dangleptr commented Jun 28, 2019 • edited Loading

dangleptr commented Jul 4, 2019

nebula-community-bot commented Jul 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebula-community-bot commented Jul 12, 2019

dangleptr commented Jul 15, 2019

nebula-community-bot commented Jul 15, 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

nebula-community-bot commented Jul 15, 2019

wadeliuyi left a comment

Choose a reason for hiding this comment

nebula-community-bot commented Jul 15, 2019

whitewum 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

nebula-community-bot commented Jul 15, 2019

dangleptr commented Jul 15, 2019

wadeliuyi left a comment

Choose a reason for hiding this comment

nebula-community-bot commented Jul 15, 2019

dangleptr commented Jun 28, 2019 •

edited

Loading