Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16709 )
Change subject: [test] fix tablet_copy_client-test not work for multi-thread DownloadBlocks status collect ...................................................................... Patch Set 3: Code-Review+1 (4 comments) Overall looks good, just a few nits. http://gerrit.cloudera.org:8080/#/c/16709/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16709/3//COMMIT_MSG@7 PS3, Line 7: [test] fix tablet_copy_client-test not work for multi-thread DownloadBlocks status collect In Kudu, we are striving to keep the summary short :) See https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines that is referenced from https://kudu.apache.org/docs/contributing.html#_submitting_patches page. In this context, how about changing this summary to: [tablet_copy_client-test] fix scenario on DownloadBlocks() error reporting http://gerrit.cloudera.org:8080/#/c/16709/3//COMMIT_MSG@9 PS3, Line 9: In patch : https://gerrit.cloudera.org/#/c/16274/8/src/kudu/tserver/tablet_copy_client-test.cc, : the error status collect cannot be test properly. : Currently the code aimed at testing whether parallelized version of DownloadBlocks() : can collect error status correctly or not. : However, it only called StartCopy() but no DownloadBlocks(). How about slightly updating this description to become something like below: In patch [1] the status reporting of the multi-threaded DownloadBlocks() was not tested properly. The original test scenario called only StartCopy(), but DownloadBlocks() was not called. This patch moves the corresponding test code into a separate scenario, targeting testing failure paths in any of the download threads spawned from TabletCopyClient::DownloadBlocks(). With this patch, both StartCopy() and DownloadBlocks() are called in the scenario. [1] https://gerrit.cloudera.org/#/c/16274/8/src/kudu/tserver/tablet_copy_client-test.cc http://gerrit.cloudera.org:8080/#/c/16709/3/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: http://gerrit.cloudera.org:8080/#/c/16709/3/src/kudu/tserver/tablet_copy_client-test.cc@294 PS3, Line 294: Test error status can be collect corretly among multi threads. How about: Test that error status is properly reported if there was a failure in any of multiple threads downloading tablet's data blocks. http://gerrit.cloudera.org:8080/#/c/16709/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/16709/3/src/kudu/tserver/tablet_copy_client.cc@788 PS3, Line 788: VLOG_WITH_PREFIX(1) << "Downloading block with block_id " << old_block_id.ToString(); nit: I think it's worth moving the newly added MAYBE_RETURN_FAILURE after this VLOG_WITH_PREFIX. The idea is to have the same visibility in the logs of what methods are started even if injecting faults. -- To view, visit http://gerrit.cloudera.org:8080/16709 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bfbe12bf13d0cc274ad266f62c5539f7da791b8 Gerrit-Change-Number: 16709 Gerrit-PatchSet: 3 Gerrit-Owner: wangning <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: wangning <[email protected]> Gerrit-Comment-Date: Mon, 16 Nov 2020 22:00:40 +0000 Gerrit-HasComments: Yes
