Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18374 )
Change subject: [tool] Add tool to copy replica from local filesystem ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG@21 PS7, Line 21: - Using --src_* and --dst_* prefixes to clarify what directories So just checking my understanding, the idea here is that a user should stand up a new Kudu tserver at the same machine, but mounting a different set of directories. Once set up, the user runs this tool to copy tablets from one local tserver to the other, and once complete, starts up the new tserver? I also wonder if you considered an online process for rebalancing data (eg through a maintenance manager thread, or a tweak in compaction prioritization or somesuch). http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tools/tool_action_local_replica.cc@482 PS7, Line 482: LOG(INFO) << fake_replica->last_status(); I'm not sure how much value this logging adds, over just doing it in the tool's main thread. Presumably, fake_replica->last_status() will be OK for the duration of the tool, right? And we seems like we do have some logging throughout the client's life cycle. Alternatively, maybe consider instantiating the tablet_copy_client_metrics and outputting them? http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client-test.cc@20 PS7, Line 20: nit: can drop the extra line http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.h@197 PS7, Line 197: const boost::optional<std::string>& copy_peer_uuid = boost::none); nit: why not rely on an empty string? Or pass in the local fs UUID? It's not too important, but it is a bit awkward as is, since this can be valid, invalid ("(unknown uuid)"), or none http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@352 PS7, Line 352: CHECK_OK Why not RETURN_NOT_OK? Won't the tool exit early anyway? http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1055 PS7, Line 1055: pb_util::SecureShortDebugString(session_->tablet_superblock())); nit: the superblock might be huge, and it might be difficult to figure out what the state actually is. Consider using `TabletDataState_Name(data_state)`? http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1097 PS7, Line 1097: CHECK_OK nit: RETURN_NOT_OK? I think this can fail if there's no space, so it'd be nice to fail gracefully http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_source_session.h File src/kudu/tserver/tablet_copy_source_session.h: http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_source_session.h@243 PS7, Line 243: std::string session_id_; : std::string requestor_uuid_; nit: these can both be const -- To view, visit http://gerrit.cloudera.org:8080/18374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Gerrit-Change-Number: 18374 Gerrit-PatchSet: 7 Gerrit-Owner: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Comment-Date: Mon, 11 Apr 2022 19:05:37 +0000 Gerrit-HasComments: Yes
