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 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/19620/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19620/2//COMMIT_MSG@16 PS2, Line 16: tablet_copy_download_superblock_in_batch > Currently, it uses FLAGS_tablet_copy_transfer_chunk_size_bytes to split the 1. If the superblock is larger than --rpc_max_message_size, the copy procedure will fail definitely. We have to adjust the parameters after seeing such errors. Is it possible to resolce the problem automatically? 2. What would happen if the data size is larger than --tablet_copy_transfer_chunk_size_bytes, and --tablet_copy_transfer_chunk_size_bytes is larger than --rpc_max_message_size? Would it be reasonable to use GROUP_FLAG_VALIDATOR to check --tablet_copy_transfer_chunk_size_bytes is less than --rpc_max_message_size? http://gerrit.cloudera.org:8080/#/c/19620/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19620/3//COMMIT_MSG@10 PS3, Line 10: it may over the : size of the rpc maximum message size defined as: : --rpc_max_message_size. Describe what happend then. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/kudu-tool-test.cc@9158 PS3, Line 9158: TestDownloadSuperblockInBatch It seems this test only checked the function of 'local_replica copy_from_remote', the functionality of the newly added flag --tablet_copy_download_superblock_in_batch hasn't been checked, right? http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/kudu-tool-test.cc@9164 PS3, Line 9164: tablet_ids_to_copy Check it's not empty. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/kudu-tool-test.cc@9177 PS3, Line 9177: nit: remove the space http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tools/tool_action_local_replica.cc@1370 PS3, Line 1370: .AddOptionalParameter("tablet_copy_download_threads_nums_per_session") : .AddOptionalParameter("num_threads") : .AddOptionalParameter("tablet_copy_download_superblock_in_batch") Could you please reorder them in lexicographic order? http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@325 PS3, Line 325: if (!superblock_path.empty() && : dst_fs_manager_->env()->FileExists(superblock_path)) { When the code execute here, this is always true, right? http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@328 PS3, Line 328: + nit: Use Substitute instead to keep the style. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@334 PS3, Line 334: SENSITIVE Is there any tests verify that encryption is enabled? http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@917 PS3, Line 917: uint64_t offset = 0; Move it closer to the place where use it. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@928 PS3, Line 928: NewTempRWFile Add a MakeScopedCleanup to delete the file if download failed. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@929 PS3, Line 929: RWFileOptions Set RWFileOptions.is_sensitive = true, tablet metadata is sensitive. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@944 PS3, Line 944: Error nit: error Use lower case for RETURN_NOT_OK_PREPEND. Other places are the same. http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_client.cc@950 PS3, Line 950: if (PREDICT_FALSE(FLAGS_tablet_copy_download_file_inject_latency_ms > 0)) { Is there any tests you want to inject this flag? http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/19620/3/src/kudu/tserver/tablet_copy_source_session.cc@322 PS3, Line 322: string path = fs_manager_->GetTabletMetadataPath(tablet_id_); Reading superblock from disk could not resolve the case of schema change, blocks/log segments add/remove. Use the tablet_superblock_ which is immutable after been initialized when TabletCopySourceSession Init(). -- 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: 3 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: Mon, 26 Jun 2023 11:30:01 +0000 Gerrit-HasComments: Yes
