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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sat, 12 May 2018 08:22:18 +0000 Gerrit-HasComments: Yes
