Alexey Serbin 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 3: (17 comments) Thank you for the patch! I did took a quick look, adding initial feedback. http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@7 PS3, Line 7: tool It would be great to provide an example of the usage for the tool. That could clarify on a few questions below: 1. What to do with the original source replica after it's copied? 2. What's is the behavior of the tool when starting it under a running tablet server? http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@9 PS3, Line 9: would is http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@10 PS3, Line 10: copy copying http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@10 PS3, Line 10: use used http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@11 PS3, Line 11: disk drivers data directories http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@11 PS3, Line 11: rebalance rebalancing http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@11 PS3, Line 11: add adding http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@12 PS3, Line 12: disk driver data directory http://gerrit.cloudera.org:8080/#/c/18374/3//COMMIT_MSG@12 PS3, Line 12: disk drivers them http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tablet/tablet_replica.h@115 PS3, Line 115: TabletReplica(); If this is a constructor for a limited audience, consider moving this into the private section and add corresponding 'friends' declarations. http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: PS3: Instead of introducing Mode and doing , did you http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.h@316 PS3, Line 316: client nit: to be inline with tablet_copy_source_metrics_ and dst_fs_manager_, should this be named tablet_copy_dst_metrics_ instead? http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.cc@880 PS3, Line 880: if (mode_ == TabletCopyMode::REMOTE) { : RETURN_NOT_OK_PREPEND(DownloadFile(data_id, writer.get()), : Substitute("Unable to download WAL segment with seq. number $0", : wal_segment_seqno)); : } else { : CHECK(mode_ == TabletCopyMode::LOCAL); : RETURN_NOT_OK_PREPEND(CopyFile(data_id, writer.get()), : Substitute("Unable to copy WAL segment with seq. number $0", : wal_segment_seqno)); : } nit: maybe, change this if/else to switch? http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.cc@972 PS3, Line 972: if (mode_ == TabletCopyMode::REMOTE) { : RETURN_NOT_OK_PREPEND(DownloadFile(data_id, block.get()), : Substitute("Unable to download block $0", old_block_id.ToString())); : } else { : CHECK(mode_ == TabletCopyMode::LOCAL); : RETURN_NOT_OK_PREPEND(CopyFile(data_id, block.get()), : Substitute("Unable to copy block $0", old_block_id.ToString())); : } nit: maybe, change this if/else to switch? http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_client.cc@1067 PS3, Line 1067: done = (offset + chunk_size) == total_data_length; nit: maybe, move this one line below, and then there is no need to compute (offset + chunk_size) since the offset is updated one line below. http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_service.h File src/kudu/tserver/tablet_copy_service.h: PS3: Could you separate changes on these tablet_copy_service.{h,cc} into its own changelist? That way it's much easier to track the relevant changes and review the code. http://gerrit.cloudera.org:8080/#/c/18374/3/src/kudu/tserver/tablet_copy_source_session.h File src/kudu/tserver/tablet_copy_source_session.h: PS3: Instead of introducing the TabletCopyMode enum, did you consider an option to separate a base class and have two derived classes for local and remote tablet copying sessions? -- 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: 3 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: Fri, 01 Apr 2022 19:18:39 +0000 Gerrit-HasComments: Yes
