You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/05/30 07:17:34 UTC
[kudu-CR] WIP [kudu CLI] more integration tests for rebalancer
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10540
Change subject: WIP [kudu CLI] more integration tests for rebalancer
......................................................................
WIP [kudu CLI] more integration tests for rebalancer
Added one more integration test for the rebalancer functionality
in the kudu CLI.
WIP: more tests are coming
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 214 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/1
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#5).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration tests for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 516 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/5
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#3).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration test for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalance.cc
2 files changed, 424 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/3
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jul 2018 00:30:56 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 11: Verified+1
Unrelated flake in ToolTest.TestHmsDowngrade
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Jul 2018 21:55:23 +0000
Gerrit-HasComments: No
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#6).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration tests for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 532 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/6
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] more integration tests for rebalancer
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] more integration tests for rebalancer
......................................................................
Patch Set 9:
(1 comment)
Added sharding to kudu-admin-test to speed it up when run by dist-test, in lieu of splitting it up into different files.
http://gerrit.cloudera.org:8080/#/c/10540/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10540/9//COMMIT_MSG@11
PS9, Line 11:
Added list of new tests.
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Jun 2018 20:10:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#12).
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
[tools] extra integration tests for the rebalancer
This patch adds more integration tests for the rebalancer,
providing coverage for the following scenarios:
* a new tablet server added post-rebalancing,
and the rebalancer is run again
* DDL operations are run concurrently with the rebalancing
* two rebalancers are running concurrently
* a tablet server goes down during rebalancing
* a tablet server is added during rebalancing
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 610 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/12
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] more integration tests for rebalancer
......................................................................
Patch Set 8:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/10540/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10540/5//COMMIT_MSG@7
PS5, Line 7: kudu CLI
> [tools] is standard, I think.
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1374
PS5, Line 1374:
> useful
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1522
PS5, Line 1522:
> Why this instead of ASSERT_OK? You're consistent about it in these tests.
Ah, that's because ASSERT_OK() does not do the following:
ASSERT_OK(s) << s.ToString() << ":" << err;
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1533
PS5, Line 1533: RunKuduTool(tool_args, &out, &err);
> Should this also be parametrized on 3-4-3?
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1594
PS5, Line 1594:
> dynamic
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1595
PS5, Line 1595: ete_tab
> dynamic
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1711
PS5, Line 1711:
> get stuck
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1712
PS5, Line 1712: m
> a
Done
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1765
PS5, Line 1765:
> But we might expect a bad status in s?
Yep, it can be like that -- e.g., if due to some conditions the rebalancer didn't able to make progress for too long (there is --max_staleness_interval_sec for that). I added a comment on that.
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 06 Jun 2018 08:21:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 10:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1539
PS10, Line 1539: ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
: << "stderr: " << err;
> No need to check this again.
Done
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1690
PS10, Line 1690: do_create_tables = false;
: do_alter_tables = false;
: do_delete_tables = false;
> Not sure I see the point of separating these. Wouldn't a single CountDownLa
Using separate atomics per corresponding thread seemed simpler to me without bringing in unneeded sync-wait primitives. But if using CountDownLatch looks more elegant -- sure, let's do that.
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1711
PS10, Line 1711: timer.join();
: timer_cleanup.cancel();
> The use of another thread just to do timing seems unnecessarily complex. Ho
The main purpose of this is to allow the rebalancer to run concurrently with those DDL operations threads. Having the rebalancer running in the main thread is beneficial in case of errors, if any -- at least the test would not crash and it's easier to debug it in that case.
I added corresponding comment, trying to clarify on that.
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1750
PS10, Line 1750: if (!AllowSlowTests()) {
: LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run";
: return;
: }
:
: const auto is_343_scheme = (GetParam() == Kudu1097::Enable);
: constexpr int kRepFactor = 3;
: constexpr int kNumTservers = 2 * (kRepFactor + 1);
: constexpr int kNumTables = 10;
: const string table_name_pattern = "rebalance_test_table_$0";
: constexpr int kTserverUnresponsiveMs = 3000;
: const vector<string> kMasterFlags = {
: Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme),
: Substitute("--tserver_unresponsive_timeout_ms=$0", kTserverUnresponsiveMs),
: };
: const vector<string> kTserverFlags = {
: Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme),
: };
:
: FLAGS_num_tablet_servers = kNumTservers;
: FLAGS_num_replicas = kRepFactor;
: NO_FATALS(BuildAndStart(kTserverFlags, kMasterFlags));
:
: ASSERT_OK(CreateUnbalancedTables(
: cluster_.get(), client_.get(), schema_, table_name_pattern, kNumTables,
: kRepFactor, kRepFactor + 1, kNumTservers, kTserverUnresponsiveMs));
> This is common to all of these new tests; can it be shared?
Done
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1808
PS10, Line 1808: // Run another rebalancer concurrently in the main test thread.
> Hmm, is this enough concurrency to be worthwhile? How long does each rebala
The idea is to make sure the cluster state would be consistent and rebalancers would not crush if running concurrently. In essence, if each rebalancer follows the same greedy algorithm with minimal variations (which is true), not many rebalancers are necessary to spot a problem here, if any. However, I agree it makes sense to have more and start them 'more synchronously'.
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1887
PS10, Line 1887: const auto sleep_sec = 5 + (r.Next() % 3);
> Seems like we've picked a particular range of sleeping durations that we th
Yep, that was about having some 'precise' timing. The idea was that the desired timing would be so for almost all runs, and that seems to be true even with dist-test and TSAN builds.
Flipping a tserver on/off is not exactly what is needed here, and I'm not sure it's possible to track the 'inacceptable health status UNAVAILABLE' error in that case reliably at all.
I updated the scenario to rely on the state of the cluster that 'kudu ksck' sees when the rebalancer runs, making this scenario much more robust.
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1902
PS10, Line 1902: "--max_moves_per_server=2",
> Why is this flag important?
Done
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1975
PS10, Line 1975: SleepFor(MonoDelta::FromSeconds(3 + (r.Next() % 3)));
> See above; this also seems too delicately chosen to be robust.
Done
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 02 Jul 2018 21:26:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
[tools] extra integration tests for the rebalancer
This patch adds more integration tests for the rebalancer,
providing coverage for the following scenarios:
* a new tablet server added post-rebalancing,
and the rebalancer is run again
* DDL operations are run concurrently with the rebalancing
* two rebalancers are running concurrently
* a tablet server goes down during rebalancing
* a tablet server is added during rebalancing
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Reviewed-on: http://gerrit.cloudera.org:8080/10540
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 610 insertions(+), 47 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#9).
Change subject: [tools] more integration tests for rebalancer
......................................................................
[tools] more integration tests for rebalancer
Added more integration tests for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 547 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/9
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#8).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration tests for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 533 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/8
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
Patch Set 7:
(10 comments)
Just some nits I forgot to post earlier...otherwise looks good.
http://gerrit.cloudera.org:8080/#/c/10540/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10540/5//COMMIT_MSG@7
PS5, Line 7: kudu CLI
[tools] is standard, I think.
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1374
PS5, Line 1374:
useful
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1522
PS5, Line 1522:
Why this instead of ASSERT_OK? You're consistent about it in these tests.
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1533
PS5, Line 1533:
Should this also be parametrized on 3-4-3?
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1594
PS5, Line 1594:
dynamic
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1595
PS5, Line 1595:
dynamic
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1711
PS5, Line 1711:
get stuck
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1712
PS5, Line 1712: )
a
http://gerrit.cloudera.org:8080/#/c/10540/5/src/kudu/tools/kudu-admin-test.cc@1765
PS5, Line 1765: ing err;
But we might expect a bad status in s?
http://gerrit.cloudera.org:8080/#/c/10540/7/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/7/src/kudu/tools/kudu-admin-test.cc@1568
PS7, Line 1568: // The idea is to wait until the rebalancer starts.
Looks like this belongs above L1565.
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 05 Jun 2018 18:50:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 11:
(4 comments)
What did you learn after looping the new tests?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1711
PS10, Line 1711: }
: }
> The main purpose of this is to allow the rebalancer to run concurrently wit
I agree that running the rebalancer in the main thread is nicer, but how is a SleepFor() between the two rebalancer runs any different than timer.join()? In both cases the main thread is blocked waiting for time to elapse, except the former is more direct than the latter.
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1831
PS11, Line 1831: auto concurrent_runners_cleanup = MakeScopedCleanup([&]() {
: start_synchronizer.CountDown();
: for (auto& runner : concurrent_runners) {
: runner.join();
: }
: });
:
: // Run rebalancers concurrently.
: start_synchronizer.CountDown();
:
: // Wait for all rebalancers to complete.
: for (auto& runner : concurrent_runners) {
: runner.join();
: }
: concurrent_runners_cleanup.cancel();
This is a strange pattern; it duplicates the cleanup code outright. Why is it necessary? I don't see anything that can fail between the creation of the scoped cleanup and the execution of the duplicated cleanup code, so what purpose does the scoped cleanup serve?
Would it help if ScopedCleanup provided a way to execute the cleanup code now and prevent it from running again?
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1930
PS11, Line 1930: RunTimeError
Nit: RuntimeError
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@2029
PS11, Line 2029: RunTimeError
Nit: RuntimeError.
Also, can you decompose this into a helper and use it in the other test too?
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Jul 2018 21:48:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 11:
Thank you for the review.
> (4 comments)
>
> What did you learn after looping the new tests?
I learned that there should be some additional handling of transient errors related to RaftConsensus in the tool, e.g. failures related to CAS, connection refused (when a tablet server shuts down), and backpressure-related errors. Otherwise, it's necessary to restart the tool once such an error happens, but it would be nice if the tool handle that automatically and exit only when the cluster is balanced or some non-transient error happens.
The absence of handling of those transient errors lead to some flakiness in particular scenarios if running with --stress-cpu-threads=16, but the rate of failure is low (1/50 or so).
Not a big deal, but I'm thinking of adding corresponding code for that.
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Jul 2018 22:02:24 +0000
Gerrit-HasComments: No
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
Patch Set 3: Verified+1
(1 comment)
Unrelated flake: Check failed: false Found non-HEALTHY LEADER. It seems there is upstream JIRA case for that, but I could not find it.
http://gerrit.cloudera.org:8080/#/c/10540/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/1/src/kudu/tools/kudu-admin-test.cc@78
PS1, Line 78: using kudu::client::KuduSchemaBuilder;
> warning: using decl 'KuduTable' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 31 May 2018 17:57:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1711
PS10, Line 1711: timer.join();
: timer_cleanup.cancel();
> I agree that running the rebalancer in the main thread is nicer, but how is
It's a bit different if the first run of the rebalancer takes longer then 30 seconds. Basically, I don't want to depend on the rebalancer runtime if it sees the need to continue the rebalancing over and over again when it detects some tables appear/disappear during the on-going DDL activity.
In real life, I think it's OK if the rebalancer continues to run in case of ongoing DDL activity if it spots the need for additional rebalancing cycle. However, for the test I don't want big timing variations depending on ongoing DDL activity.
I added a comment, trying to clarify on that point.
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1831
PS11, Line 1831: string err;
: const auto s = RunKuduTool(tool_args, &out, &err);
: ASSERT_TRUE(s.ok()) << s.ToString() << ": " << err;
: ASSERT_STR_CONTAINS(out,
: "rebalancing is complete: cluster is balanced (moved 0 replicas)")
: << "stderr: " << err;
: }
:
: NO_FATALS(cluster_->AssertNoCrashes());
: NO_FATALS(ClusterVerifier(cluster_.get()).CheckCluster());
: }
:
: // The rebalancer should stop and exit upon detecting a tablet server that
: // went down. That's a simple and effective way of preventing concurrent replica
: // movement by the rebalancer and the
> This is a strange pattern; it duplicates the cleanup code outright. Why is
Yes, indeed -- that's a good observation.
I wrote this code following the generic pattern with ScopedCleanup, which is common for all other places when some error might happen between the finalization code (waiting for the threads to join) and the place when the ScopedCleanup declared.
Yep, in fact, given the current logic, no ScopedCleanup is necessary -- I'll remove that.
I think in some cases it might be useful if the ScopedCleanup could run its code not only during the destruction phase. But I looked around and it seems there are only a few places where it's used like this, so maybe it's not worth adding a new method into ScopedCleanup.
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1930
PS11, Line 1930: "test is sk
> Nit: RuntimeError
Done
http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@2029
PS11, Line 2029:
> Nit: RuntimeError.
Done
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Jul 2018 23:48:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] extra integration tests for the rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#11).
Change subject: [tools] extra integration tests for the rebalancer
......................................................................
[tools] extra integration tests for the rebalancer
This patch adds more integration tests for the rebalancer,
providing coverage for the following scenarios:
* a new tablet server added post-rebalancing,
and the rebalancer is run again
* DDL operations are run concurrently with the rebalancing
* two rebalancers are running concurrently
* a tablet server goes down during rebalancing
* a tablet server is added during rebalancing
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 619 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/11
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] WIP [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#2).
Change subject: WIP [kudu CLI] more integration tests for rebalancer
......................................................................
WIP [kudu CLI] more integration tests for rebalancer
Added one more integration test for the rebalancer functionality
in the kudu CLI.
WIP: more tests are coming
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalance.cc
2 files changed, 356 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/2
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [tools] Extra integration tests for the rebalancer
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] Extra integration tests for the rebalancer
......................................................................
Patch Set 10:
(8 comments)
These new tests are pretty complex; can you make sure they're robust by looping them in dist-test, both with and without TSAN, and with stress threads?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1539
PS10, Line 1539: ASSERT_STR_CONTAINS(out, "rebalancing is complete: cluster is balanced")
: << "stderr: " << err;
No need to check this again.
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1690
PS10, Line 1690: do_create_tables = false;
: do_alter_tables = false;
: do_delete_tables = false;
Not sure I see the point of separating these. Wouldn't a single CountDownLatch be better? The nice thing about a latch is that the threads could sleep and check the latch at the same time with WaitFor().
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1711
PS10, Line 1711: timer.join();
: timer_cleanup.cancel();
The use of another thread just to do timing seems unnecessarily complex. How about capturing MonoTime::Now() before the first rebalancer run, setting up a deadline using that value plus 30s, then waiting for that deadline here?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1750
PS10, Line 1750: if (!AllowSlowTests()) {
: LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run";
: return;
: }
:
: const auto is_343_scheme = (GetParam() == Kudu1097::Enable);
: constexpr int kRepFactor = 3;
: constexpr int kNumTservers = 2 * (kRepFactor + 1);
: constexpr int kNumTables = 10;
: const string table_name_pattern = "rebalance_test_table_$0";
: constexpr int kTserverUnresponsiveMs = 3000;
: const vector<string> kMasterFlags = {
: Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme),
: Substitute("--tserver_unresponsive_timeout_ms=$0", kTserverUnresponsiveMs),
: };
: const vector<string> kTserverFlags = {
: Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme),
: };
:
: FLAGS_num_tablet_servers = kNumTservers;
: FLAGS_num_replicas = kRepFactor;
: NO_FATALS(BuildAndStart(kTserverFlags, kMasterFlags));
:
: ASSERT_OK(CreateUnbalancedTables(
: cluster_.get(), client_.get(), schema_, table_name_pattern, kNumTables,
: kRepFactor, kRepFactor + 1, kNumTservers, kTserverUnresponsiveMs));
This is common to all of these new tests; can it be shared?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1808
PS10, Line 1808: // Run another rebalancer concurrently in the main test thread.
Hmm, is this enough concurrency to be worthwhile? How long does each rebalancer take to run? Should we spin up more than 2 at the same time?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1887
PS10, Line 1887: const auto sleep_sec = 5 + (r.Next() % 3);
Seems like we've picked a particular range of sleeping durations that we think is precisely enough to stop a tserver mid-rebalance. That isn't particularly robust; what if it's a TSAN environment where everything is moving more slowly, and we end up shutting down the tserver well before the rebalance starts? Or after it ends?
What if instead we flipped the same tserver on and off repeatedly during the rebalance? I think that'd increase the chances of an overlap. Would it prevent the test from passing?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1902
PS10, Line 1902: "--max_moves_per_server=2",
Why is this flag important?
http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1975
PS10, Line 1975: SleepFor(MonoDelta::FromSeconds(3 + (r.Next() % 3)));
See above; this also seems too delicately chosen to be robust.
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Jun 2018 23:36:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#4).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration test for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 516 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/4
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Extra integration tests for the rebalancer
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#10) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] Extra integration tests for the rebalancer
......................................................................
[tools] Extra integration tests for the rebalancer
This patch adds more integration tests for the rebalancer:
- Coverage for adding a new tablet server post-rebalancing and then
rebalancing again.
- Coverage for DDL concurrent with rebalancing.
- Coverage for running two rebalancers concurrently.
- Coverage for when a tablet server goes down during rebalancing.
- Coverage for when a tablet server is added during rebalancing.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 549 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/10
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [kudu CLI] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10540
to look at the new patch set (#7).
Change subject: [kudu CLI] more integration tests for rebalancer
......................................................................
[kudu CLI] more integration tests for rebalancer
Added more integration tests for the rebalancer functionality
in the kudu CLI.
Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 533 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/10540/7
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] more integration tests for rebalancer
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] more integration tests for rebalancer
......................................................................
Patch Set 9:
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/13737/ : FAILURE
It seems it's better to split those tests, putting the new rebalancer tests into a separate bundle (i.e. file).
--
To view, visit http://gerrit.cloudera.org:8080/10540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8
Gerrit-Change-Number: 10540
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 06 Jun 2018 15:27:27 +0000
Gerrit-HasComments: No