Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20282 )
Change subject: KUDU-3498 Scanner keeps alive in periodically ...................................................................... Patch Set 6: (19 comments) http://gerrit.cloudera.org:8080/#/c/20282/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20282/4//COMMIT_MSG@9 PS4, Line 9: Kudu caches the scanner in the tablet server fo > In fact, Kudu tablet servers run those scanner objects when service scan re You are right. http://gerrit.cloudera.org:8080/#/c/20282/4//COMMIT_MSG@12 PS4, Line 12: is every large, it takes a long time to handle it > Just FYI: another way of handling that is setting smaller batch size by cal Got it. http://gerrit.cloudera.org:8080/#/c/20282/4//COMMIT_MSG@14 PS4, Line 14: will be expired even if it has sent a keep alive request > There is no sense in sending KeepAlive request at the same time (e.g., righ You are right. The user can certainly start a new thread to send KeepAlive() request by self. But it is better to provide this feature by Kudu client. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2964 PS4, Line 2964: > It would be great to add a test to verify how the new functionality works i Thanks for you comments. The new unit tests will solve these two test scenario. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2965 PS4, Line 2965: INSTANTIATE_TEST_SUITE_P(KeepAlivePeriodically, KeepAlivePeriodicallyTest, ::testing::Bool()); > Since this test scenario assumes running for at least 3 seconds, it's consi Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2967 PS4, Line 2967: vePeriodicallyTest, TestSca > non-replicated table Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2972 PS4, Line 2972: // Create a new cluster with 3 tablet servers. > nit: add a period (.) in the end of the sentence Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2974 PS4, Line 2974: > Why the second tablet is important here? How do you even know that multipl It is a wrong description. Deleted it. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2986 PS4, Line 2986: _bound1->SetInt32("key", INT32_MIN)); > make sure the scanner expires when keep-alive requests are not sent When keep_alive_periodically is false, keep-alive requests are not sent and the scanner expires. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2993 PS4, Line 2993: rtialRow> lower_ > has expired Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client-test.cc@2994 PS4, Line 2994: SERT_OK(lower_bound2->SetInt > nit: when using this sort of assert, consider adding s.ToString() -- that w Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/client.h@2957 PS4, Line 2957: > building a messenger failed Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.h@277 PS4, Line 277: // Send keep alive requests periodically in an independent thread. > Please add a small doc comment (as the other methods in this class have). Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.h@302 PS4, Line 302: > nit: please keep the DISALLOW_COPY_AND_ASSIGN() macro as the last element i Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.h@303 PS4, Line 303: ts periodically. > nit: please add a comment at least to describe when this member becomes non Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc@104 PS4, Line 104: KeepAlivePeriodically > What happens with the periodic timer when KuduScanner::Data::OpenTablet() i When the scanner opens next tablet, KeepAlivePeriodically() will use the new scanner id to send keep-alive requests. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc@109 PS4, Line 109: keep_alive_timer_ > Do we need to do anything with this member field upon an explicit call of t When the object of KuduScanner is released, it will call deconstruction function of ~Data() and keep_alive_timer_ will be stopped. http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc@110 PS4, Line 110: std::move > nit: use std::move() here? Done http://gerrit.cloudera.org:8080/#/c/20282/4/src/kudu/client/scanner-internal.cc@115 PS4, Line 115: Start > Does it make sense to stop sending keep-alive requests for the scanner if a Function KeepAlive() had done this check. And KeepAlivePeriodically() calls KeepAlive() periodically. It should be stopped when the scanner is released. -- To view, visit http://gerrit.cloudera.org:8080/20282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1165d96814eb4bcd5db9b5cb60403fffc5b18c81 Gerrit-Change-Number: 20282 Gerrit-PatchSet: 6 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Mon, 28 Aug 2023 06:05:08 +0000 Gerrit-HasComments: Yes
