You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/02 19:03:29 UTC

[kudu-CR] WIP: KUDU-1735. Fix crash when aborting skipping config change round 

Todd Lipcon has uploaded a new change for review.

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

Change subject: WIP: KUDU-1735. Fix crash when aborting skipping config change round
......................................................................

WIP: KUDU-1735. Fix crash when aborting skipping config change round

WIP because I need some tests to repro this

Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
---
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus_state.cc
3 files changed, 59 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................

KUDU-1735. Fix crash when aborting a skipped config change round

This fixes a crash seen in a test cluster when deleting a tablet
in which there is a pending (uncommitted) REPLICATE message for a
skipped CONFIG_CHANGE operation. The CONFIG_CHANGE was skipped because
the tablet already had a higher indexed configuration written to disk.

This can happen in a couple cases: one, exercised by a test included
here, is if a tablet server crashes just after committing a config
change to disk but before writing the COMMIT message to the WAL, then
upon restart that config change will be a pending operation despite being
committed. If the table is then deleted before the operation is
committed, it would crash on abort.

The approach taken to fix this is as follows:

When aborting a config change operation, we only clear the 'pending'
config if we see that its index matches the index of the operation being
aborted. Otherwise, we ignore the abort (whereas we used to crash).

In order to achieve this, we have to remember the index of the pending
config, which previously wasn't set until after the commit. So, this
assignment is moved out of the commit path and into
AddPendingOperationUnlocked(). Changing the assumption of where the
index was set involved making a few corresponding changes to DCHECKs
elsewhere in the code.

There is a slight wire/upgrade compatibility issue here: previously the
leader would replicate config change messages with no opid set in the
'new_config'. Now, it does. I think this would cause DCHECKs to fire in
a mixed-version cluster, though no CHECKs. Additionally, we aren't
purporting to fully support rolling upgrade yet, so I don't think it's a
big deal. I was able to upgrade the test cluster which experienced this
issue in-place and it came up fine.

This patch removes raft_consensus_state-test, which had some various
assertions against setting pending configuration changes with opids
set. After looking at this test, I realized that it's purporting to test
ReplicaState but in fact all of the methods that it tests are just
pass-throughs to ConsensusMeta and thus are already covered by
consensus_meta-test. So, I figured there's no sense spending time
updating this redundant test code.

I ran raft_consensus-itest 1000 times[1]and the new test alone another
1000 times each in DEBUG[2] and TSAN[3] to test.

[1] http://dist-test.cloudera.org/job?job_id=todd.1478141668.26073
[2] http://dist-test.cloudera.org/job?job_id=todd.1478291990.1887
[3] http://dist-test.cloudera.org/job?job_id=todd.1478292124.2771

Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
D src/kudu/consensus/raft_consensus_state-test.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
8 files changed, 119 insertions(+), 175 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4916/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS2, Line 610: ->
> man that's a lot of arrows. maybe we could just get the mutable pointer bel
Done


http://gerrit.cloudera.org:8080/#/c/4916/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 237:       pending_(Substitute("T $0 P $1 ", options.tablet_id, peer_uuid)),
> nit: maybe we should add ": " instead of " "
changed to ": "


PS3, Line 619: const RaftConfigPB& new_config = change_record.new_config();
> I forget: do we have checks somewhere that the new config has a higher term
the config only stores its index, and yea there are already checks to ignore it if it has a lower index


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


KUDU-1735. Fix crash when aborting a skipped config change round

This fixes a crash seen in a test cluster when deleting a tablet
in which there is a pending (uncommitted) REPLICATE message for a
skipped CONFIG_CHANGE operation. The CONFIG_CHANGE was skipped because
the tablet already had a higher indexed configuration written to disk.

This can happen in a couple cases: one, exercised by a test included
here, is if a tablet server crashes just after committing a config
change to disk but before writing the COMMIT message to the WAL, then
upon restart that config change will be a pending operation despite being
committed. If the table is then deleted before the operation is
committed, it would crash on abort.

The approach taken to fix this is as follows:

When aborting a config change operation, we only clear the 'pending'
config if we see that its index matches the index of the operation being
aborted. Otherwise, we ignore the abort (whereas we used to crash).

In order to achieve this, we have to remember the index of the pending
config, which previously wasn't set until after the commit. So, this
assignment is moved out of the commit path and into
AddPendingOperationUnlocked(). Changing the assumption of where the
index was set involved making a few corresponding changes to DCHECKs
elsewhere in the code.

There is a slight wire/upgrade compatibility issue here: previously the
leader would replicate config change messages with no opid set in the
'new_config'. Now, it does. I think this would cause DCHECKs to fire in
a mixed-version cluster, though no CHECKs. Additionally, we aren't
purporting to fully support rolling upgrade yet, so I don't think it's a
big deal. I was able to upgrade the test cluster which experienced this
issue in-place and it came up fine.

This patch removes raft_consensus_state-test, which had some various
assertions against setting pending configuration changes with opids
set. After looking at this test, I realized that it's purporting to test
ReplicaState but in fact all of the methods that it tests are just
pass-throughs to ConsensusMeta and thus are already covered by
consensus_meta-test. So, I figured there's no sense spending time
updating this redundant test code.

I ran raft_consensus-itest 1000 times[1]and the new test alone another
1000 times each in DEBUG[2] and TSAN[3] to test.

[1] http://dist-test.cloudera.org/job?job_id=todd.1478141668.26073
[2] http://dist-test.cloudera.org/job?job_id=todd.1478291990.1887
[3] http://dist-test.cloudera.org/job?job_id=todd.1478292124.2771

Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Reviewed-on: http://gerrit.cloudera.org:8080/4916
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
D src/kudu/consensus/raft_consensus_state-test.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
8 files changed, 119 insertions(+), 175 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................

KUDU-1735. Fix crash when aborting a skipped config change round

This fixes a crash seen in a test cluster when deleting a tablet
in which there is a pending (uncommitted) REPLICATE message for a
skipped CONFIG_CHANGE operation. The CONFIG_CHANGE was skipped because
the tablet already had a higher indexed configuration written to disk.

This can happen in a couple cases: one, exercised by a test included
here, is if a tablet server crashes just after committing a config
change to disk but before writing the COMMIT message to the WAL. If it's
then removed from the configuration and restarted, the config change
operation will stay pending until the tablet is eventually deleted, at
which point the crash will be triggered. The newly added test provokes
this condition and fails prior to the included bug fix.

The approach taken to fix this is as follows:

When aborting a config change operation, we only clear the 'pending'
config if we see that its index matches the index of the operation being
aborted. Otherwise, we ignore the abort (whereas we used to crash).

In order to achieve this, we have to remember the index of the pending
config, which previously wasn't set until after the commit. So, this
assignment is moved out of the commit path and into
AddPendingOperationUnlocked(). Changing the assumption of where the
index was set involved making a few corresponding changes to DCHECKs
elsewhere in the code.

There is a slight wire/upgrade compatibility issue here: previously the
leader would replicate config change messages with no opid set in the
'new_config'. Now, it does. I think this would cause DCHECKs to fire in
a mixed-version cluster, though no CHECKs. Additionally, we aren't
purporting to fully support rolling upgrade yet, so I don't think it's a
big deal. I was able to upgrade the test cluster which experienced this
issue in-place and it came up fine.

This patch removes raft_consensus_state-test, which had some various
assertions against setting pending configuration changes with opids
set. After looking at this test, I realized that it's purporting to test
ReplicaState but in fact all of the methods that it tests are just
pass-throughs to ConsensusMeta and thus are already covered by
consensus_meta-test. So, I figured there's no sense spending time
updating this redundant test code.

I ran raft_consensus-itest 1000 times[1] and the new test alone another
1000 times[2] to test.

[1] http://dist-test.cloudera.org/job?job_id=todd.1478141668.26073
[2] http://dist-test.cloudera.org/job?job_id=todd.1478141941.26651

Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
D src/kudu/consensus/raft_consensus_state-test.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
7 files changed, 120 insertions(+), 175 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

Just nits. ignore/don't ignore, you choose :)

http://gerrit.cloudera.org:8080/#/c/4916/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS2, Line 610: ->
man that's a lot of arrows. maybe we could just get the mutable pointer below?


http://gerrit.cloudera.org:8080/#/c/4916/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

PS3, Line 619: const RaftConfigPB& new_config = change_record.new_config();
I forget: do we have checks somewhere that the new config has a higher term and/or index? should we?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................

KUDU-1735. Fix crash when aborting a skipped config change round

This fixes a crash seen in a test cluster when deleting a tablet
in which there is a pending (uncommitted) REPLICATE message for a
skipped CONFIG_CHANGE operation. The CONFIG_CHANGE was skipped because
the tablet already had a higher indexed configuration written to disk.

This can happen in a couple cases: one, exercised by a test included
here, is if a tablet server crashes just after committing a config
change to disk but before writing the COMMIT message to the WAL, then
upon restart that config change will be a pending operation despite being
committed. If the table is then deleted before the operation is
committed, it would crash on abort.

The approach taken to fix this is as follows:

When aborting a config change operation, we only clear the 'pending'
config if we see that its index matches the index of the operation being
aborted. Otherwise, we ignore the abort (whereas we used to crash).

In order to achieve this, we have to remember the index of the pending
config, which previously wasn't set until after the commit. So, this
assignment is moved out of the commit path and into
AddPendingOperationUnlocked(). Changing the assumption of where the
index was set involved making a few corresponding changes to DCHECKs
elsewhere in the code.

There is a slight wire/upgrade compatibility issue here: previously the
leader would replicate config change messages with no opid set in the
'new_config'. Now, it does. I think this would cause DCHECKs to fire in
a mixed-version cluster, though no CHECKs. Additionally, we aren't
purporting to fully support rolling upgrade yet, so I don't think it's a
big deal. I was able to upgrade the test cluster which experienced this
issue in-place and it came up fine.

This patch removes raft_consensus_state-test, which had some various
assertions against setting pending configuration changes with opids
set. After looking at this test, I realized that it's purporting to test
ReplicaState but in fact all of the methods that it tests are just
pass-throughs to ConsensusMeta and thus are already covered by
consensus_meta-test. So, I figured there's no sense spending time
updating this redundant test code.

I ran raft_consensus-itest 1000 times[1]and the new test alone another
1000 times each in DEBUG[2] and TSAN[3] to test.

[1] http://dist-test.cloudera.org/job?job_id=todd.1478141668.26073
[2] http://dist-test.cloudera.org/job?job_id=todd.1478291990.1887
[3] http://dist-test.cloudera.org/job?job_id=todd.1478292124.2771

Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
D src/kudu/consensus/raft_consensus_state-test.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
8 files changed, 121 insertions(+), 175 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

Added a nit but the change looks good

http://gerrit.cloudera.org:8080/#/c/4916/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 237:       pending_(Substitute("T $0 P $1 ", options.tablet_id, peer_uuid)),
nit: maybe we should add ": " instead of " "


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 3:

This is ready for a review (ping Mike/David)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1735. Fix crash when aborting a skipped config change round

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1735. Fix crash when aborting a skipped config change round
......................................................................


Patch Set 3:

This looks pretty good to me. I want to do one more pass on it tomorrow.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No