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/06/06 05:07:25 UTC

[kudu-CR] [tools] 'auto' mode for tablet movements for RF=1 

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10612


Change subject: [tools] 'auto' mode for tablet movements for RF=1
......................................................................

[tools] 'auto' mode for tablet movements for RF=1

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  The criterion is based on
the version of Kudu masters and tablet servers
(to track fix KUDU-2443) and replica management scheme.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer.cc
M src/kudu/tools/tool_action_cluster.cc
3 files changed, 210 insertions(+), 16 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/1
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 13: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>
Gerrit-Comment-Date: Wed, 04 Jul 2018 05:10:59 +0000
Gerrit-HasComments: No

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed a vote on this change.

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 7
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#12).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies.  The fix is present
whenever the version is larger than 1.7.0.

This patch also contains a unit test for the newly introduced
Is343SchemeCluster() utility function and an integration test scenario
RebalancerAndSingleReplicaTablets.  Also, already existing
RebalanceParamTest provides additional coverage for the functionality
in case of --move_single_replicas=auto (the default setting).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
7 files changed, 526 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/12
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#6) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
4 files changed, 165 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/6
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 6
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#4) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_version_util.cc
M src/kudu/tools/tool_version_util.h
4 files changed, 166 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/4
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 4
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#11).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies.  The fix is present
whenever the version is larger than 1.7.0.

This patch also contains a unit test for the newly introduced
Is343SchemeCluster() utility function and an integration test scenario
RebalancerAndSingleReplicaTablets.  Also, already existing
RebalanceParamTest provides additional coverage for the functionality
in case of --move_single_replicas=auto (the default setting).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
7 files changed, 486 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/11
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#5) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
4 files changed, 170 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/5
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 5
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@55
PS5, Line 55: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@59
PS5, Line 59: using std::make_tuple;
> warning: using decl 'make_tuple' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@60
PS5, Line 60: using std::set_intersection;
> warning: using decl 'set_intersection' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@62
PS5, Line 62: using std::sort;
> warning: using decl 'sort' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@65
PS5, Line 65: using std::unordered_map;
> warning: using decl 'unordered_map' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/10612/5/src/kudu/tools/tool_action_cluster.cc@66
PS5, Line 66: using std::unordered_set;
> warning: using decl 'unordered_set' is unused [misc-unused-using-decls]
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 5
Gerrit-Owner: 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, 13 Jun 2018 06:49:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#9).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

The existing RebalanceParamTest provides coverage in case of
--move_single_replicas=auto (the default setting for the flag).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
6 files changed, 298 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/9
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 9
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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/10612/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10612/3//COMMIT_MSG@10
PS3, Line 10:   
nit: Fixed two spaces.


http://gerrit.cloudera.org:8080/#/c/10612/3//COMMIT_MSG@10
PS3, Line 10: based on
            : the version
Added guide to behavior per version to the commit msg.


http://gerrit.cloudera.org:8080/#/c/10612/3//COMMIT_MSG@12
PS3, Line 12:  
nit: Added "the".


http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/kudu-admin-test.cc@1464
PS3, Line 1464: "--move_single_replicas=enabled"
This isn't necessary with this patch, and serves as a light test that the version detection is working.


http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/tool_action_cluster.cc@116
PS3, Line 116: if (std::find(allowed_values.begin(), allowed_values.end(), flag_value) !=
             :       allowed_values.end()) {
             :     return true;
             :   }
Adjusted to be case-insensitive.


http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/tool_action_cluster.cc@152
PS3, Line 152: ksck_result
nit: ksck_results


http://gerrit.cloudera.org:8080/#/c/10612/3/src/kudu/tools/tool_action_cluster.cc@235
PS3, Line 235: auto ver_str_all_good = true;
             :   string ver_str_offending;
This version stuff will be pulled into a different patch and put in a different file. While it will hopefully be rare, this should give us a sane way to manage enabling or disabling capabilities of tools based on the versions in the cluster the tool is operating on.



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 3
Gerrit-Owner: 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, 12 Jun 2018 17:40:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 11:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc@1510
PS11, Line 1510:   const auto& param = GetParam();
               :   const auto& move_single_replica = std::get<0>(param);
               :   const auto is_343_scheme = (std::get<1>(param) == Kudu1097::Enable);
               :   constexpr auto kRepFactor = 1;
               :   constexpr auto kNumTservers = 2 * (kRepFactor + 1);
               :   constexpr auto kNumTables = kNumTservers;
               :   const string table_name_pattern = "rebalance_test_table_$0";
               :   constexpr auto 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),
               :   };
> Nit: can you sort this a bit?
Done


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc@1560
PS11, Line 1560: (kRepFactor + 1)
> Nit: don't need parens.
Done


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-tool-test.cc@3364
PS11, Line 3364: signle
> Shouldn't this be 'single'?
Done


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.h
File src/kudu/tools/tool_replica_util.h:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.h@132
PS11, Line 132: no information available to find on the replica management scheme
              : // used
> "not enough information available to determine the replica management schem
Done


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc
File src/kudu/tools/tool_replica_util.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc@524
PS11, Line 524: signle
> typo here
Done


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc@539
PS11, Line 539:     if (tablet_id.empty()) {
              :       return Status::Incomplete(Substitute(
              :           "table '$0': empty tablet identifier for one of the tablets",
              :           table_name));
              :     }
> Is this even possible?
Nope, I think it's not possible.  I removed that -- anyway the GetTabletLeader() will handle it appropriately.



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>
Gerrit-Comment-Date: Wed, 04 Jul 2018 00:27:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#8).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
2 files changed, 172 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/8
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 11:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc@1510
PS11, Line 1510:   const auto& param = GetParam();
               :   const auto& move_single_replica = std::get<0>(param);
               :   const auto is_343_scheme = (std::get<1>(param) == Kudu1097::Enable);
               :   constexpr auto kRepFactor = 1;
               :   constexpr auto kNumTservers = 2 * (kRepFactor + 1);
               :   constexpr auto kNumTables = kNumTservers;
               :   const string table_name_pattern = "rebalance_test_table_$0";
               :   constexpr auto 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),
               :   };
Nit: can you sort this a bit?

1. constexpr stuff
2. const foo kBar stuff
3. const auto under_score_stuff

Maybe like that?


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-admin-test.cc@1560
PS11, Line 1560: (kRepFactor + 1)
Nit: don't need parens.


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/kudu-tool-test.cc@3364
PS11, Line 3364: signle
Shouldn't this be 'single'?


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.h
File src/kudu/tools/tool_replica_util.h:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.h@132
PS11, Line 132: no information available to find on the replica management scheme
              : // used
"not enough information available to determine the replica management scheme"


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc
File src/kudu/tools/tool_replica_util.cc:

http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc@524
PS11, Line 524: signle
typo here


http://gerrit.cloudera.org:8080/#/c/10612/11/src/kudu/tools/tool_replica_util.cc@539
PS11, Line 539:     if (tablet_id.empty()) {
              :       return Status::Incomplete(Substitute(
              :           "table '$0': empty tablet identifier for one of the tablets",
              :           table_name));
              :     }
Is this even possible?



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>
Gerrit-Comment-Date: Tue, 03 Jul 2018 22:51:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 10: Verified+1

(3 comments)

Unrelated flake in RaftConsensusITest.TestElectionMetrics

http://gerrit.cloudera.org:8080/#/c/10612/9/src/kudu/tools/rebalance-test.cc
File src/kudu/tools/rebalance-test.cc:

http://gerrit.cloudera.org:8080/#/c/10612/9/src/kudu/tools/rebalance-test.cc@215
PS9, Line 215:                const vector<KsckResultsTestConfig>& test_configs) {
> warning: invalid case style for parameter 'rebalancerCfg' [readability-iden
Done


http://gerrit.cloudera.org:8080/#/c/10612/9/src/kudu/tools/rebalance-test.cc@216
PS9, Line 216:     for (auto idx = 0; idx < test_configs.size(); ++idx) {
> warning: invalid case style for parameter 'testConfigs' [readability-identi
Done


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: 
> It just seemed odd to me that the earlier version of this patch came close 
Ah, sure -- that makes sense.  I added a test which verifies the behavior of the Is343SchemeCluster() utility function.



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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 01:10:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#13).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies.  The fix is present
whenever the version is larger than 1.7.0.

This patch also contains a unit test for the newly introduced
Is343SchemeCluster() utility function and an integration test scenario
RebalancerAndSingleReplicaTablets.  Also, already existing
RebalanceParamTest provides additional coverage for the functionality
in case of --move_single_replicas=auto (the default setting).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
7 files changed, 528 insertions(+), 96 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/13
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 7: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 7
Gerrit-Owner: 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: Fri, 15 Jun 2018 17:33:24 +0000
Gerrit-HasComments: No

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: 
> Indeed -- it should return false if it's 3-2-3.  It seems like a typo.
Was there a test to cover this? Seems like there should have been; otherwise the existing code may have been merged.


http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc@244
PS8, Line 244: take long time
Nit: take a long time


http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc@246
PS8, Line 246: available
Nit: be available



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 8
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: Sat, 23 Jun 2018 16:36:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 13:

> Patch Set 13: Code-Review+2

Thank you for the review!  I re-based the patch and resolved a couple of trivial conflicts.


-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>
Gerrit-Comment-Date: Wed, 04 Jul 2018 05:29:36 +0000
Gerrit-HasComments: No

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: 
> Was there a test to cover this? Seems like there should have been; otherwis
I'm not sure there was a test to verify Is343SchemeCluster() functionality.  Probably, I need to add a new one.

Just to clarify: what do we want to test, actually?


http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc@244
PS8, Line 244: take long time
> Nit: take a long time
Done


http://gerrit.cloudera.org:8080/#/c/10612/8/src/kudu/tools/tool_action_cluster.cc@246
PS8, Line 246: available
> Nit: be available
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 8
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, 27 Jun 2018 19:42:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#10).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

This patch also contains a unit test for the newly introduced
Is343SchemeCluster() utility function.

The existing RebalanceParamTest provides coverage in case of
--move_single_replicas=auto (the default setting for the flag).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
7 files changed, 386 insertions(+), 37 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/10
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies.  The fix is present
whenever the version is larger than 1.7.0.

This patch also contains a unit test for the newly introduced
Is343SchemeCluster() utility function and an integration test scenario
RebalancerAndSingleReplicaTablets.  Also, already existing
RebalanceParamTest provides additional coverage for the functionality
in case of --move_single_replicas=auto (the default setting).

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Reviewed-on: http://gerrit.cloudera.org:8080/10612
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/rebalance-test.cc
M src/kudu/tools/rebalancer.h
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_replica_util.cc
M src/kudu/tools/tool_replica_util.h
7 files changed, 528 insertions(+), 96 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 14
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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#7) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1. This is determined by examining the
version of Kudu and the replica management schema in the cluster, which
indicates whether the fix for KUDU-2443 applies. The fix applies
whenever the version is larger than 1.7.0.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/util/version_util.cc
M src/kudu/util/version_util.h
4 files changed, 164 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/7
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 7
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 12: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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:31:54 +0000
Gerrit-HasComments: No

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

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/10612

to look at the new patch set (#2).

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................

[rebalancer] 'auto' mode for RF=1 tablet movements

Added logic to automatically determine whether the tool should
move replicas of tablets with RF=1.  The criterion is based on
the version of Kudu masters and tablet servers
(to track KUDU-2443 fix) and current replica management scheme.

Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
2 files changed, 221 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/2
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 2
Gerrit-Owner: 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] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 8:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@101
PS7, Line 101:  'disabled'. The value of 'auto' means "
             :               "turn it on/off depending on the replica m
> This can be implied using the right flag tag (perhaps advanced or experimen
Yep, it could be implied using the flag tags, but I'm not sure it's applicable in case of the flag for the 'kudu' utility.  I think we can just drop this ominous sentence.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@173
PS7, Line 173:                            client->default_admin_operation_timeout(),
> Nit: add an inline arg to explain what this nullptr means.
Done


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@180
PS7, Line 180:   Version v;
             :   if (!ParseVersion(version_str, &v).ok()) {
> Nit: I think this would be simpler as a single DCHECK on "disabled", on L18
Done


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@220
PS7, Line 220: summaries }) {
             :     for (const auto& summary : summaries) {
             :       if (summary.version) {
             :         if (!VersionSupportsRF1Movement(*summary.version)) {
> I'll be honest; I don't understand this explanation. Could you clarify furt
Indeed: the comment does not make much sense.  Updated.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@224
PS7, Line 224:           LOG(INFO) << "found Kudu server of version '" << *summary.version
> Nit: what's the point of this extra scope?
Done


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: 
> Don't we have to test this too? If not, why bother setting it to non-null?
Indeed -- it should return false if it's 3-2-3.  It seems like a typo.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h
File src/kudu/util/version_util.h:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h@60
PS7, Line 60: 
> I don't think this belongs here. This is util code, so one would expect the
Done


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc@85
PS7, Line 85: 
> Nit: drop
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 8
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: Sat, 23 Jun 2018 01:58:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
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>

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: 
> I'm not sure there was a test to verify Is343SchemeCluster() functionality.
It just seemed odd to me that the earlier version of this patch came close to being merged upstream with this glaring bug. Seems like it should have been caught via a test. To be specific, there should be a test that sets up a cluster without 3-4-3 and verifies that it's illegal to move an RF=1 tablet in that cluster. Such a test would pass now but fail if run with the earlier version of this patch.



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 9
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, 27 Jun 2018 22:58:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )

Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
......................................................................


Patch Set 7:

(8 comments)

Can you articulate how the existing tests provide sufficient coverage for --move_single_replicas=auto?

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@101
PS7, Line 101: It's best to keep the default value for this "
             :               "flag unless you know what you are doing."
This can be implied using the right flag tag (perhaps advanced or experimental).


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@173
PS7, Line 173:                            nullptr, is_343_scheme);
Nit: add an inline arg to explain what this nullptr means.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@180
PS7, Line 180:     DCHECK(boost::iequals(FLAGS_move_single_replicas, "enabled") ||
             :            boost::iequals(FLAGS_move_single_replicas, "disabled"));
Nit: I think this would be simpler as a single DCHECK on "disabled", on L185.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@220
PS7, Line 220: That's because write
             :   // operations in progress (if any) cannot be committed until the destination
             :   // replica (added as a voter) is caught up. Catching up the destination
             :   // voter replica might take much longer than write operation's timeout.
I'll be honest; I don't understand this explanation. Could you clarify further?


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@224
PS7, Line 224:   {
Nit: what's the point of this extra scope?


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226: is_343_scheme
Don't we have to test this too? If not, why bother setting it to non-null?


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h
File src/kudu/util/version_util.h:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h@60
PS7, Line 60: bool VersionSupportsRF1Movement(const std::string& version_str);
I don't think this belongs here. This is util code, so one would expect the functions and classes to be Kudu-agnostic.


http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:

http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc@85
PS7, Line 85: std::
Nit: drop



-- 
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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:48:11 +0000
Gerrit-HasComments: Yes