Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9490 )
Change subject: [tools] Add a tool to recover master data from tablet servers ...................................................................... Patch Set 9: (7 comments) Just a quick initial glance. http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/kudu-admin-test.cc@3020 PS9, Line 3020: shouldnt nit: shouldn't or should not http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/kudu-admin-test.cc@3133 PS9, Line 3133: from nit: drop http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.h File src/kudu/tools/master_rebuilder.h: http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.h@35 PS9, Line 35: POD object f nit: this is not a POD type since its non-static members are not PODs. If in doubt, you can check that using the template<typename T> struct std::is_pod trait: https://en.cppreference.com/w/cpp/types/is_pod http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.h@81 PS9, Line 81: const RebuildReport& GetRebuildReport(); nit: make this method 'const'? http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.cc@63 PS9, Line 63: using master::Master; : using master::MasterOptions; : using master::SysCatalogTable; : using master::SysTablesEntryPB; : using master::SysTabletsEntryPB; : using master::TableInfo; : using master::TableMetadataLock; : using master::TabletInfo; : using master::TabletMetadataGroupLock; : using master::TabletMetadataLock; : using std::string; : using std::vector; : using strings::Substitute; : using tserver::ListTabletsResponsePB; nit: move these using declarations out from the kudu namespace http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.cc@155 PS9, Line 155: InvalidArgument Would IllegalState/Incomplete/ServiceUnavailable be better choices here? http://gerrit.cloudera.org:8080/#/c/9490/9/src/kudu/tools/master_rebuilder.cc@321 PS9, Line 321: Status s = sys_catalog.CreateNew(master.fs_manager()); As an option, would it be a safer approach to create the whole master's filesystem data directory structure in some temporary directory, perform all the necessary steps to populate the catalog with the metadata, and only in the very end move the directory into the specified destination? That way the critical interval of placing the data into the destination might be shortened. -- To view, visit http://gerrit.cloudera.org:8080/9490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If29e421d466a531ebad72e281ae27e74e458f8c6 Gerrit-Change-Number: 9490 Gerrit-PatchSet: 9 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 10 Aug 2021 21:28:48 +0000 Gerrit-HasComments: Yes
