Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19620 )
Change subject: KUDU-3459 Download superblock piece by piece ...................................................................... Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9362 PS8, Line 9362: int I guess it can be replaced by std::tuple<bool, bool>, and enum DownloadSuperblockInBatchCheckArgsType can be removed. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9445 PS8, Line 9445: tablet_ids_to_copy Because there is only 1 tablet, why use vector? http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9465 PS8, Line 9465: AddColumn(column_name)->Type(client::KuduColumnSchema::INT32) How many columns will be added to reach the threadshold? You can add extra attributes to the column to make it easier to reach the size, for example add default value, add column comments, and etc. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9467 PS8, Line 9467: vector<string> data_dirs = mini_cluster_->mini_tablet_server(src_index) : ->options()->fs_opts.data_roots; : uint64_t file_size = 0; : string superblock_path = : Substitute("$0/tablet-meta/$1", data_dirs.at(0), tablet_ids_to_copy.at(0)); You can obtain the path by calling mini_cluster_->mini_tablet_server(src_index)->server()->fs_manager()->GetTabletMetadataPath(tablet_id_to_copy) http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9481 PS8, Line 9481: mini_cluster_->mini_tablet_server(src_index) How about defining local variables "src_tserver", "dst_tserver" to simplify the code? http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9510 PS8, Line 9510: // The size of superblock is less than rpc_max_message_size, it will cause a network error. : if (test_case == kOnceTimeAndUnencrpted || test_case == kOnceTimeAndEncrpted) { : ASSERT_STR_CONTAINS(stderr, "recv error: Network error: RPC frame had a length"); : return; : } Why skip these test cases? http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy.proto File src/kudu/tserver/tablet_copy.proto: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy.proto@100 PS8, Line 100: download_superblock_in_batch Set the default value to false, or judge whether this field is set in service side. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy.proto@127 PS8, Line 127: superblock_is_too_large Set the default value to false, it's possible the service side is in older version which doesn't support download superblock by pieces, the response message doesn't have such field. http://gerrit.cloudera.org:8080/#/c/19620/6/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19620/6/src/kudu/tserver/tablet_copy_client.cc@928 PS6, Line 928: options > No. Here downloading superblock file without any format. If set is_sensitiv OK, you can use temporary variable in NewTempRWFile(). http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_client.cc@305 PS8, Line 305: if (FLAGS_tablet_copy_download_superblock_in_batch) { : req.set_download_superblock_in_batch( : FLAGS_tablet_copy_download_superblock_in_batch); : } How about setting it directly without the if-statement? Both the false and not-set state have the same behavior on service side. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_service.cc File src/kudu/tserver/tablet_copy_service.cc: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_service.cc@221 PS8, Line 221: maybe Isn't it definite? http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_service.cc@436 PS8, Line 436: DeleteTmpSuperblockFile It would be nice to hide the behavior of session in its destructor. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_source_session.cc@107 PS8, Line 107: uperblock_tmp_path_(""), Not necessary, the default value is empty string. -- To view, visit http://gerrit.cloudera.org:8080/19620 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a7c212784383e247566a4701965e2897de83303 Gerrit-Change-Number: 19620 Gerrit-PatchSet: 8 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 04 Jul 2023 03:42:08 +0000 Gerrit-HasComments: Yes
