Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19620 )
Change subject: KUDU-3459 Download superblock piece by piece ...................................................................... Patch Set 9: (14 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: tDo > I guess it can be replaced by std::tuple<bool, bool>, and enum DownloadSupe Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9445 PS8, Line 9445: > Because there is only 1 tablet, why use vector? Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9465 PS8, Line 9465: AGS_tablet_copy_auto_download_superblock_in_batch, > How many columns will be added to reach the threadshold? You can add extra Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9467 PS8, Line 9467: (kSuperblockSize / 8), : FLAGS_encrypt_data_at_rest), nullptr, &stderr); : // The size of superblock is less than rpc_max_message_size, it will cause a network error. : // Downloading superblock will fail. : if (!FLAGS_tablet_copy_auto_download_superblock_in_batch) { > You can obtain the path by calling Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9481 PS8, Line 9481: > How about defining local variables "src_tserver", "dst_tserver" to simplify Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tools/kudu-tool-test.cc@9510 PS8, Line 9510: } : auto fs_manager = mini_cluster_->mini_tablet_server(kCopyTserverIndex)->server()->fs_manager(); : string src_fs_wal_dir = fs_manager->GetWalsRootDir(); : src_fs_wal_dir = src_fs_wal_dir.substr(0, src_fs_wal_dir.length() - strlen("/wals")); : s > Why skip these test cases? Because this case will downloading superblock failed. 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: auto_download_superblock_in_ > Set the default value to false, or judge whether this field is set in servi Done 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 Done 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: req.set_auto_download_superblock_in_batch(FLAGS_tablet_copy_auto_download_superblock_in_batch); : rpc::RpcController controller; : : / > How about setting it directly without the if-statement? Both the false and Done 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@218 PS8, Line 218: ock piece by piece."; > I'm not clear what dose this flag used for, isn't it enough to control the OK. I will remove it. http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_service.cc@221 PS8, Line 221: table > Isn't it definite? Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_service.cc@436 PS8, Line 436: > It would be nice to hide the behavior of session in its destructor. Done http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_source_session.h File src/kudu/tserver/tablet_copy_source_session.h: http://gerrit.cloudera.org:8080/#/c/19620/8/src/kudu/tserver/tablet_copy_source_session.h@249 PS8, Line 249: > Is it necessary, you can judge whether "superblock_tmp_path_" is empty or n Done 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. Done -- 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: 9 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 10:18:25 +0000 Gerrit-HasComments: Yes
