You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/11/29 12:38:24 UTC

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

Hello Alexey Serbin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/8682

to review the following change.


Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report received contains a health report.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 8 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8682/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8682/1/src/kudu/master/catalog_manager.cc@3491
PS1, Line 3491:       if (FLAGS_raft_prepare_replacement_before_eviction &&
              :           !cstate.leader_uuid().empty() &&
              :           cstate.committed_config().peers_size() > 0 &&
              :           cstate.committed_config().peers(0).has_health_report()) {
              :         // An alternative scheme of managing tablet replicas: the catalog
              :         // manager processes the health-related info on replicas from the tablet
              :         // report and initiates appropriate modifications for the tablet Raft
              :         // configuration: evict an already-replaced failed voter replica or add
              :         // a new non-voter replica marked for promotion as a replacement.
              :         //
              :         // We only run this code path when receiving a leader's tablet report,
              :         // since the leader is the only node that sends a health report for the
              :         // peers.
              :         const RaftConfigPB& config = cstate.committed_config();
              :         string to_evict;
              :         if (IsUnderReplicated(config, replication_factor)) {
              :           rpcs.emplace_back(new AsyncAddReplicaTask(
              :               master_, tablet, cstate, RaftPeerPB::NON_VOTER, &rng_));
              :         } else if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
              :                    CanEvictReplica(config, replication_factor, &to_evict)) {
              :           DCHECK(!to_evict.empty());
              :           rpcs.emplace_back(new AsyncEvictReplicaTask(
              :               master_, tablet, cstate, std::move(to_evict)));
              :         }
              :       } else if (consensus_state_updated &&
              :                  FLAGS_master_add_server_when_underreplicated &&
              :                  CountVoters(cstate.committed_config()) < replication_factor) {
              :         // Add a server to the config if it is under-replicated.
              :         //
              :         // This is an idempotent operation due to a CAS enforced on the
              :         // committed config's opid_index.
              :         rpcs.emplace_back(new AsyncAddReplicaTask(
              :             master_, tablet, cstate, RaftPeerPB::VOTER, &rng_));
              :       }
I think there is a logic error in the updated code.  The FLAGS_raft_prepare_replacement_before_eviction was used as a kill switch, but now it's not.  What if FLAGS_raft_prepare_replacement_before_eviction == true and one of the newly added criteria yields false?  Then code related to old replica management scheme will be executed, but that's not what is needed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Nov 2017 19:58:55 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8682

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report was received from the leader.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 24 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report received contains a health report.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 18 insertions(+), 10 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8682

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report was received from the leader.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 24 insertions(+), 21 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc@3507
PS4, Line 3507: ts_desc->permanent_uuid() == cstate.leader_uuid()
Is it possible that both are empty?


http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc@3510
PS4, Line 3510: FLAGS_master_add_server_when_underreplicated
It's a good one.


http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc@3510
PS4, Line 3510:         if (FLAGS_master_add_server_when_underreplicated &&
              :             IsUnderReplicated(config, replication_factor)) {
              :           rpcs.emplace_back(new AsyncAddReplicaTask(
              :               master_, tablet, cstate, RaftPeerPB::NON_VOTER, &rng_));
              :         } else if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
              :                    CanEvictReplica(config, replication_factor, &to_evict)) {
              :           DCHECK(!to_evict.empty());
              :           rpcs.emplace_back(new AsyncEvictReplicaTask(
              :               master_, tablet, cstate, std::move(to_evict)));
              :         }
maybe, flip the order  of evaluation of those conditions in this changelist?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:33:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report was received from the leader.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Reviewed-on: http://gerrit.cloudera.org:8080/8682
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/master/catalog_manager.cc
1 file changed, 24 insertions(+), 20 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 7
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 02 Dec 2017 01:29:26 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8682/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8682/1//COMMIT_MSG@14
PS1, Line 14: test for this
I think we can add the patch later, sure.

Thank you for addressing the issue.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Nov 2017 20:00:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 4:

rev 3 is the intended change, rev 4 is just a rebase onto latest master


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:11:38 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/8682

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................

KUDU-1097. Only add/evict when processing leader tablet reports

The leader replica is the only replica that sends a health report. The
master needs the health report to make an eviction decision. Thus, we
should only consider eviction or addition of a new replica if the tablet
report was received from the leader.

There is no standalone test for this but this patch is required for the
following patch, "Never evict a leader", to pass due to the DCHECK
introduced in that patch.

Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 19 insertions(+), 16 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8682/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8682/1/src/kudu/master/catalog_manager.cc@3491
PS1, Line 3491:       if (FLAGS_raft_prepare_replacement_before_eviction &&
              :           !cstate.leader_uuid().empty() &&
              :           cstate.committed_config().peers_size() > 0 &&
              :           cstate.committed_config().peers(0).has_health_report()) {
              :         // An alternative scheme of managing tablet replicas: the catalog
              :         // manager processes the health-related info on replicas from the tablet
              :         // report and initiates appropriate modifications for the tablet Raft
              :         // configuration: evict an already-replaced failed voter replica or add
              :         // a new non-voter replica marked for promotion as a replacement.
              :         //
              :         // We only run this code path when receiving a leader's tablet report,
              :         // since the leader is the only node that sends a health report for the
              :         // peers.
              :         const RaftConfigPB& config = cstate.committed_config();
              :         string to_evict;
              :         if (IsUnderReplicated(config, replication_factor)) {
              :           rpcs.emplace_back(new AsyncAddReplicaTask(
              :               master_, tablet, cstate, RaftPeerPB::NON_VOTER, &rng_));
              :         } else if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
              :                    CanEvictReplica(config, replication_factor, &to_evict)) {
              :           DCHECK(!to_evict.empty());
              :           rpcs.emplace_back(new AsyncEvictReplicaTask(
              :               master_, tablet, cstate, std::move(to_evict)));
              :         }
              :       } else if (consensus_state_updated &&
              :                  FLAGS_master_add_server_when_underreplicated &&
              :                  CountVoters(cstate.committed_config()) < replication_factor) {
              :         // Add a server to the config if it is under-replicated.
              :         //
              :         // This is an idempotent operation due to a CAS enforced on the
              :         // committed config's opid_index.
              :         rpcs.emplace_back(new AsyncAddReplicaTask(
              :             master_, tablet, cstate, RaftPeerPB::VOTER, &rng_));
              :       }
> I think there is a logic error in the updated code.  The FLAGS_raft_prepare
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 30 Nov 2017 00:44:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097. Only add/evict when processing leader tablet reports

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

Change subject: KUDU-1097. Only add/evict when processing leader tablet reports
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc@3507
PS4, Line 3507: ts_desc->permanent_uuid() == cstate.leader_uuid()
> Is it possible that both are empty?
no, i don't think so, at least not in production, but i'll add that check


http://gerrit.cloudera.org:8080/#/c/8682/4/src/kudu/master/catalog_manager.cc@3510
PS4, Line 3510:         if (FLAGS_master_add_server_when_underreplicated &&
              :             IsUnderReplicated(config, replication_factor)) {
              :           rpcs.emplace_back(new AsyncAddReplicaTask(
              :               master_, tablet, cstate, RaftPeerPB::NON_VOTER, &rng_));
              :         } else if (PREDICT_TRUE(FLAGS_catalog_manager_evict_excess_replicas) &&
              :                    CanEvictReplica(config, replication_factor, &to_evict)) {
              :           DCHECK(!to_evict.empty());
              :           rpcs.emplace_back(new AsyncEvictReplicaTask(
              :               master_, tablet, cstate, std::move(to_evict)));
              :         }
> maybe, flip the order  of evaluation of those conditions in this changelist
ok



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If7e318e042cd27ff4544bc5aae76db4ab9e51da1
Gerrit-Change-Number: 8682
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 23:15:28 +0000
Gerrit-HasComments: Yes