Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10061 )
Change subject: [tools] ksck improvements [5/n]: Checks for experimental, unsafe, hidden flags ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/10061/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10061/5//COMMIT_MSG@30 PS5, Line 30: rolled by --truncate_server_csv_length. : Additionally, if all ch > nit: mind including this in the above snippet too? The two are far away from each other in the ksck output so I made a second snippet. http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck.cc@200 PS5, Line 200: tatus s = master->FetchInfo().AndThen([&]() { : return master->FetchConsensusState(); : }); : > nit: revert formatting? Done http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck.cc@369 PS5, Line 369: // Failing to gather flags is only a warning. > why isn't this part simply above the lock instead of moving the lock to a n Because the first and most important part is filling out the summary. The flags is ordered after as it's a secondary concern. If the lock part is below the flags part, it looks like the flags part has something to do with the locking, which it doesn't. http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc@94 PS4, Line 94: InternalMiniClusterOptions opts; : > Oh huh. I thought IMCs reflected all in-process updates of flags. Those don My bad. You are right. We will see the flag change. It may or may not affect the behavior of the server, and that's irrelevant here, which is why I made the mistake of thinking we should set it before start. http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc@229 PS4, Line 229: ASSERT_OK(ksck_->CheckMasterHealth()); : ASSERT_OK(ksck_->CheckMasterUnusualFlags()); : ASSERT_OK(ksck_->CheckClusterRunning()); : ASSERT_OK(ksck_->FetchTableAndTabletInfo()); : ASSERT_OK(ksck_->FetchInfoFromTabletServers()); : ASSERT_OK(ksck_->CheckTabletServerUnusualFlags()); : ASSERT_OK(ksck_->PrintResults()); : > Ah, I would've thought CheckMasterUnusualFlags() and CheckTabletServerUnusu Done http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_remote-test.cc@95 PS5, Line 95: > can we define a test flag instead? moving this flag to stable would break t If the flag moves to stable we pick a new flag. I don't think it's worth adding a new flag to the server just for these tests. http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_results.cc@138 PS5, Line 138: DCHECK_LE(servers.size(), server_count); > flags are usually defined the same way on each server, especially when a cl Good idea! http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_results.cc@267 PS5, Line 267: > Since these are warnings, let's just print `s.message()`. That way we won't Done -- To view, visit http://gerrit.cloudera.org:8080/10061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd6c179e5256b2f2bae2f7486c5e0365ef184706 Gerrit-Change-Number: 10061 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 25 May 2018 19:00:22 +0000 Gerrit-HasComments: Yes
