You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2018/05/03 16:09:08 UTC

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck 

Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10293


Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetcvhInfoFromTabletServers() checked for this Status, and
this resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit introduces Status::UuidMismatch() which is used for this
purpose instead and can be used for similar UUID mismatch errors in the
future.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
8 files changed, 20 insertions(+), 4 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() and
FetchConsensusState() which can be used to add further context to a
non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 62 insertions(+), 29 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h@28
PS8, Line 28: #include "kudu/util/net/net_util.h"
> yep, declared it in ksck.h, thanks for the tip
IWYU didn't like it, moved it to ksck_remote.h instead.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 11:11:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:       return Status::UuidMismatch(Substitute("ID reported by tablet server ($0) doesn't "
would rather introduce some new enum specific to ksck tablet server status instead of a new general-purpose Status



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 03 May 2018 18:09:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 12:

(2 comments)

Just the two nits and LGTM.

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck.cc@296
PS12, Line 296: ,&
nit: Missing space.


http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc@156
PS12, Line 156: DCHECK
nit: It's CHECK elsewhere.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 22 May 2018 16:41:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 14: Code-Review+2

Leaving this open in case Alexey or Will have more feedback


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 24 May 2018 15:13:33 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() and
FetchConsensusState() which can be used to add further context to a
non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 60 insertions(+), 28 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() and
FetchConsensusState() which can be used to add further context to a
non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 60 insertions(+), 28 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 27 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 9:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc@296
PS8, Line 296: ts->FetchInfo(&health).AndThen([&ts,&health]() {
             :                 if (FLAGS_consensus) {
             :                   return ts->FetchConsensusState(&h
> Ah, looking here, I don't think `health` gets set if `FetchConsensusState()
Updated FetchConsensusState() to FetchConsensusState(&health), this felt more natural, + this way we can introduce other statuses later, for example CONSENSUS_FETCH_FAILED instead of the more generic UNAVAILABLE. This felt out of scope though.


http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h@28
PS8, Line 28: #include "kudu/util/net/net_util.h"
> We should be able to forward declare KsckServerHealth, no?
yep, declared it in ksck.h, thanks for the tip



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 11:01:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc@156
PS12, Line 156: DCHECK
> nit: It's CHECK elsewhere.
where else do you mean? I changed these to DCHECK based on Alexey's suggestion on the previous patch set btw.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 23 May 2018 15:23:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() which
can be used to add further context to a non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 40 insertions(+), 25 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@101
PS4, Line 101: KsckServerHealth*
> Just for code hygiene, could you add a CHECK(health) here to ensure health 
Done


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@129
PS4, Line 129: 
> nit: "these variables".
Done


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@477
PS4, Line 477: }
> My mistake, I overlooked ksck_remote-test.cc! Feel free to ignore my sugges
I agree that adding a regression test wouldn't hurt here, but I'm not sure how to start it. KUDU-2426 was triggered by unauthorized requests which I'm not sure how/if I can do with the mini_cluster. In raft_consensus-itest.cc the RemoteError is triggered by memory pressure, which also doesn't seem viable for ksck tests.

Can you think of a way that I should test this?


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h@276
PS4, Line 276: 
> Does returning a non-OK status mean that health is guaranteed to not be HEA
Andrew, it does now, thanks for the tip. Also, nullptr must not be nullptr and it's now documented.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc@155
PS4, Line 155: KsckServerHealth* healt
> Also need to handle the nullptr case.
Done


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc@166
PS4, Line 166: ring response_uuid = resp.status().node_instan
> Why don't we return an error here anymore?
For some reason I thought this shouldn't be an error if it doesn't have its own status, I didn't realize I could still use RemoteError, just needed to make sure not to use WRONG_SERVER_UUID status for every RemoteError... fixing this



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 12 May 2018 08:22:18 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 27 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

Yea, I dont think adding a new Status is the right approach here from a compatibility perspective.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 03 May 2018 18:08:51 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc@156
PS12, Line 156: DCHECK
> where else do you mean? I changed these to DCHECK based on Alexey's suggest
Just in FetchInfo in ksck-test, which is a test file so a different situation. Please disregard.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 23 May 2018 15:54:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 4:

(1 comment)

> Patch Set 2:
> 
> (1 comment)

thanks for the .

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:     if (response_uuid != uuid()) {
> What do you think about changing the signature of FetchInfo() to:
thanks for the tip. as for the masters, it's a good idea, but I'd rather do it in a separate patch as currently there's no WRONG_SERVER_UUID for this at all so it feels weird to add it in this bugfix patch. what do you think?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 12:48:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 11:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc@296
PS8, Line 296: ts->FetchInfo(&health).AndThen([&ts,&health]() {
             :                 if (FLAGS_consensus) {
             :                   return ts->FetchConsensusState(&h
> Sound good. Mind adding it to the commit message as well?
Done


http://gerrit.cloudera.org:8080/#/c/10293/10/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/10/src/kudu/tools/ksck_remote.cc@209
PS10, Line 209: *health = KsckServerHealth::UNAVAILABLE;
> This never gets set to HEALTHY. I'm surprised that isn't caught somewhere b
nice catch. I looked into it, the tests didn't catch it as ksck-test.cc uses a mock FetchConsensusState(KsckServerHealth* health) which always returns KsckServerHealth::OK and ksck_remote-test.cc doesn't check the whole consensus matrix, only checked for WRONG_SERVER_UUID and as FetchConsensusState is called only if FetchInfo returns OK.

Now there's a new assertion in that fails RemoteKsckTest.TestTabletServerMismatchedUUID if FetchConsensusState(KsckServerHealth* health) returns UNAVAILABLE.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:57:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() which
can be used to add further context to a non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 33 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() which
can be used to add further context to a non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 34 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck.cc@296
PS12, Line 296: , 
> nit: Missing space.
Done


http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/12/src/kudu/tools/ksck_remote.cc@156
PS12, Line 156: DCHECK
> Just in FetchInfo in ksck-test, which is a test file so a different situati
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 24 May 2018 09:06:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@477
PS4, Line 477: TEST_F(KsckTest, TestWrongUUIDTabletServer) {
Hmm.. maybe also add a regression test specific to KUDU-2426 that other remote errors don't end up with WRONG_SERVER_UUID?

Also it'd be nice if we had an integration test that actually tested the RemoteTabletServer's behavior, maybe using the ClusterVerifier and a mini cluster? TestRestartWithDifferentUUID in raft_consensus-itest.cc might be worth checking out.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h@276
PS4, Line 276: // Connects to the configured tablet server and populates the fields of this class.
> Document if 'health' is allowed to be nullptr, and if it is, what the seman
Does returning a non-OK status mean that health is guaranteed to not be HEALTHY? Similarly, if this returns OK, does that mean health is guaranteed to be HEALTHY? I think it'd be nice if both of these were true, although guaranteeing them might take some work.

If not, I at least agree with Will, the new semantics of this function should be documented.


http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:     if (response_uuid != uuid()) {
> We have an expected uuid known ahead of time, from an authority, for each t
Ah, I meant changing the signature of the KsckMaster::FetchInfo() to also return server health (ie UNAVAILABLE). But I agree, it belongs in a separate patch if anything.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc@166
PS4, Line 166: *health = KsckServerHealth::WRONG_SERVER_UUID;
Why don't we return an error here anymore?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 22:19:18 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 14: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 24 May 2018 16:01:31 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() and
FetchConsensusState() which can be used to add further context to a
non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Reviewed-on: http://gerrit.cloudera.org:8080/10293
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 62 insertions(+), 29 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved
  Alexey Serbin: Looks good to me, but someone else must approve

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_remote-test.cc
File src/kudu/tools/ksck_remote-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_remote-test.cc@241
PS6, Line 241: $0
> nit: Could you reorder the substitutions so $0 occurs as the first unique s
Done


http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_results.cc
File src/kudu/tools/ksck_results.cc:

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_results.cc@301
PS6, Line 301: $0
> Not your fault, but could you renumber these from $0 and remove the unused 
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 15 May 2018 10:35:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit introduces Status::UuidMismatch() which is used for this
purpose instead and can be used for similar UUID mismatch errors in the
future.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.proto
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
8 files changed, 20 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@477
PS4, Line 477: TEST_F(KsckTest, TestWrongUUIDTabletServer) {
> Hmm.. maybe also add a regression test specific to KUDU-2426 that other rem
My mistake, I overlooked ksck_remote-test.cc! Feel free to ignore my suggestion.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 11 May 2018 02:49:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 10:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc@296
PS8, Line 296: ts->FetchInfo(&health).AndThen([&ts,&health]() {
             :                 if (FLAGS_consensus) {
             :                   return ts->FetchConsensusState(&h
> Updated FetchConsensusState() to FetchConsensusState(&health), this felt mo
Sound good. Mind adding it to the commit message as well?


http://gerrit.cloudera.org:8080/#/c/10293/10/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/10/src/kudu/tools/ksck_remote.cc@209
PS10, Line 209: *health = KsckServerHealth::UNAVAILABLE;
This never gets set to HEALTHY. I'm surprised that isn't caught somewhere by test; shouldn't that mean it will always result in an UNAVAILABLE ts in the report?

Also, for symmetry, doc `health` in ksck.h and check for nullptr.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 15:42:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 11:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck.h@284
PS11, Line 284: this class. 'health' must
              :   // not be nullptr.
Does it output anything with the 'health' out parameter?  I think it's worth documenting that as well.

BTW, why not to put the 'health' as a member of the class as well?


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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck.cc@298
PS11, Line 298: return ts->FetchConsensusState(&health);
Why is it necessary to overwrite health status with FetchConsensusState()?  Could it change since call of FetchInfo()?


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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck_remote.cc@156
PS11, Line 156: CHECK(health)
If you added this check for this method, maybe it's worth keeping the things consistent and add corresponding CHECK() into FetchConsensusState() as well?

From other side, it will crash anyway even without that check at line 158.  Maybe, it's worth dropping this check or converting it into DCHECK()?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 18:38:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@477
PS4, Line 477:     EXTRACT_ARRAY_CHECK_SIZE(r, cstate, "vote
> I agree that adding a regression test wouldn't hurt here, but I'm not sure 
Hrm, I can't think of a great way; a somewhat naive but targeted unit test would be to create a RemoteError for the summary and ensure it doesn't get logged as WRONG_SERVER_UUID, though I'm not sure how useful that would be. I'm fine leaving the tests as they are.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h@276
PS4, Line 276: 
> Andrew, it does now, thanks for the tip. Also, nullptr must not be nullptr 
Looking good! Thanks for making that change


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

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck.cc@296
PS8, Line 296: ts->FetchInfo(&health).AndThen([&ts]() {
             :                 if (FLAGS_consensus) {
             :                   return ts->FetchConsensusState();
Ah, looking here, I don't think `health` gets set if `FetchConsensusState()` returns an error, so if it fails, we might end up with a bad server, but it would be considered HEALTHY, which I don't think seems right?

In which case, maybe we should _also_ update `FetchConsensusState()` to `FetchConsensusState(&health)`? Or set the health in the not-OK to UNAVAILABLE if it's still HEALTHY?


http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h
File src/kudu/tools/ksck_remote.h:

http://gerrit.cloudera.org:8080/#/c/10293/8/src/kudu/tools/ksck_remote.h@28
PS8, Line 28: #include "kudu/tools/ksck_results.h"
We should be able to forward declare KsckServerHealth, no?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 00:15:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() which
can be used to add further context to a non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 33 insertions(+), 20 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:                           "could not get status from server");
> Ah, I meant changing the signature of the KsckMaster::FetchInfo() to also r
Raised KUDU-2436 for this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 12 May 2018 08:31:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() and
FetchConsensusState() which can be used to add further context to a
non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 48 insertions(+), 26 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

I wasn't sure adding a new Status for this is the best idea from a forward-compatibility standpoint. For now the server doesn't respond with this status though, the Status is returned on the client side in ksck_remote.cc.

Adar, could you chime in on the possible compatibility issue?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 03 May 2018 16:14:16 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 6:

(2 comments)

Couple nits, then SGTM.

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_remote-test.cc
File src/kudu/tools/ksck_remote-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_remote-test.cc@241
PS6, Line 241: $2
nit: Could you reorder the substitutions so $0 occurs as the first unique substitution, then $1, etc?


http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_results.cc
File src/kudu/tools/ksck_results.cc:

http://gerrit.cloudera.org:8080/#/c/10293/6/src/kudu/tools/ksck_results.cc@301
PS6, Line 301: $1
Not your fault, but could you renumber these from $0 and remove the unused s.uuid argument?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 14 May 2018 23:14:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

(1 comment)

> Patch Set 2:
> 
> (1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:       return Status::UuidMismatch(Substitute("ID reported by tablet server ($0) doesn't "
> would rather introduce some new enum specific to ksck tablet server status 
that makes sense, although I'm not sure how this should be implemented. Ksck::FetchInfoFromTabletServers() currently returns Status which still makes sense for RPC/transport-level failures. Should I just return std::pair<Status, TabletServerStatus> where TabletServerStatus would be a new enum with OK, WRONG_UUID for now?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 May 2018 18:37:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 6:

> Patch Set 6: Verified-1
> 
> Build Failed 
> 
> http://jenkins.kudu.apache.org/job/kudu-gerrit/13424/ : FAILURE

test failures seem random flakes to me


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 12 May 2018 10:16:56 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 11: Code-Review+1

The test failures all seem to be clock sync errors, although it seems IWYU is also complaining about the includes in ksck_remote-test.cc


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 17:37:55 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 12:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck.h@284
PS11, Line 284: this class. 'health' must
              :   // not be nullptr.
> Documented it. Should it be a member of KsckTabletServer instead of an out 
Hm.. I could go either way on this, although if we introduce a `health_` private member, maybe it's probably also worth thinking about how the semantics of `is_healthy()` will change, if at all.

Will, do you have a preference either way?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 17 May 2018 18:00:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@101
PS4, Line 101: 345;
Just for code hygiene, could you add a CHECK(health) here to ensure health is not nullptr? Alternatively, the function could not set the output parameter if nullptr is passed.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck-test.cc@129
PS4, Line 129: 
nit: "these variables".


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck.h@276
PS4, Line 276: // Connects to the configured tablet server and populates the fields of this class.
Document if 'health' is allowed to be nullptr, and if it is, what the semantics are in that case (probably, it is just not set as an output param in that case).


http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:       return Status::UuidMismatch(Substitute("ID reported by tablet server ($0) doesn't "
> thanks for the tip. as for the masters, it's a good idea, but I'd rather do
We have an expected uuid known ahead of time, from an authority, for each tserver- the uuid known to the leader master. We do not have authoritative uuids for the masters, just the addresses from the command line, so it's tricker to claim a master has the wrong uuid. The test that comes to mind is that if a leader master expected UUID A but the master reports UUID B, it's an imposter. We can't do that check in FetchInfo. We also can't do it if there's no leader master.

Agree the WRONG_UUID_LOGIC for masters should be another patch.


http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/4/src/kudu/tools/ksck_remote.cc@155
PS4, Line 155: 
Also need to handle the nullptr case.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 18:24:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

http://gerrit.cloudera.org:8080/#/c/10293/2/src/kudu/tools/ksck_remote.cc@165
PS2, Line 165:       return Status::UuidMismatch(Substitute("ID reported by tablet server ($0) doesn't "
> that makes sense, although I'm not sure how this should be implemented. Ksc
What do you think about changing the signature of FetchInfo() to:

 Status FetchInfo(KsckServerHealth* health);

The conditional logic in ksck.cc to select WRONG_SERVER_UUID vs UNAVAILABLE vs HEALTHY could then be done here instead. The same could be done for the masters (not necessary, but might be nice for symmetry). This also messes up some other logic in ksck.cc but nothing that can't be worked around. Curious if Will has any thoughts on this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 03:24:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................


Patch Set 12:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck.h@284
PS11, Line 284: this class. 'health' must
              :   // not be nullptr.
> Does it output anything with the 'health' out parameter?  I think it's wort
Documented it. Should it be a member of KsckTabletServer instead of an out parameter?


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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck.cc@298
PS11, Line 298: return ts->FetchConsensusState(&health);
> Why is it necessary to overwrite health status with FetchConsensusState()? 
It can't change between these two calls, but if FetchInfo() sets health to OK and FLAGS_consensus is true, then FetchConsensusState() can still set this to UNAVAILABLE if it returns a non-OK Status. Does this make sense?


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

http://gerrit.cloudera.org:8080/#/c/10293/11/src/kudu/tools/ksck_remote.cc@156
PS11, Line 156: DCHECK(health
> If you added this check for this method, maybe it's worth keeping the thing
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 20:35:26 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2426 Fix WRONG SERVER UUID case in ksck

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2426 Fix WRONG_SERVER_UUID case in ksck
......................................................................

KUDU-2426 Fix WRONG_SERVER_UUID case in ksck

RemoteKsckTabletServer::FetchInfo() returned a Status::RemoteError since
KUDU-2364 to indicate a UUID mismatch.
Ksck::FetchInfoFromTabletServers() checked for this Status, and this
resulted in any RemoteError showing as a WRONG_SERVER_UUID in the
tablet server health list.

This commit adds KsckServerHealth output argument to FetchInfo() which
can be used to add further context to a non-OK Status.

Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
7 files changed, 40 insertions(+), 25 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b4f50fe4dd94450b4f2e34dbad315bd761b071f
Gerrit-Change-Number: 10293
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>