Yingchun Lai 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 8: (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 > a user should stand up a new Kudu tserver at the same machine No need to start a real server, actually use Kudu CLI to format a new filesystem layout is OK. And after local replica copied, use the new ---src_fs_data_dirs and --dst_fs_wal_dir to start the server. In real usage scenario, we have to copy all replicas, i.e. use the tool to copy replica one by one (we can improve the tool to copy all replicas on a tserver in the future). Online process for rebalancing data between data dirs is a good idea, we can implement it after 'local tablet copy' supportted(and anchor Logs and Blocks is needed then). 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 to The main usage of this log is it willl show the copy progress, for a large size tablet, we can see and estimate how long it cost and will cost. The status is not Status but a std::string, e.g. 'Downloading block $0 ($1/$2)', 'Downloading WAL segment with seq. number $0 ($1/$2)' 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: #include <cstdint> > nit: can drop the extra line Done 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: > nit: why not rely on an empty string? Or pass in the local fs UUID? Done 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: > Why not RETURN_NOT_OK? Won't the tool exit early anyway? Done http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1055 PS7, Line 1055: Substitute("Source tablet is not ready itself! state: $0", > nit: the superblock might be huge, and it might be difficult to figure out Improved, also L313. http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1097 PS7, Line 1097: > nit: RETURN_NOT_OK? I think this can fail if there's no space, so it'd be n Done 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: const std::string session_id_; : const std::string requestor_ > nit: these can both be const Done -- 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: 8 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 12 Apr 2022 15:16:49 +0000 Gerrit-HasComments: Yes
