Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13573 )
Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table ...................................................................... Patch Set 5: (2 comments) Emm, the failed test does not seem to be caused by this patch. Looks like the issue of the leader election, maybe we need to create another ISSUE to fix it. http://gerrit.cloudera.org:8080/#/c/13573/4/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/13573/4/src/kudu/client/client-internal.h@148 PS4, Line 148: // Get the table information identified by 'table_identifier'. > Ah, you don't need to use the Doxygen style for internal functions. > That's only necessary for stuff in the public API (i.e. client.h > and a few other headers). You can use a more informal style here. Ok, I got it. How about this modification? http://gerrit.cloudera.org:8080/#/c/13573/4/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: http://gerrit.cloudera.org:8080/#/c/13573/4/src/kudu/integration-tests/tablet_history_gc-itest.cc@128 PS4, Line 128: KuduClient* client) -> Status > Either pass this by cref or pass a raw pointer; since you're not > moving the reference within the lambda, there's no need to copy it > (and thus increase/decrease the ref count). You are right. Thank you for carefully review. :) -- To view, visit http://gerrit.cloudera.org:8080/13573 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <oclarms....@gmail.com> Gerrit-Comment-Date: Wed, 12 Jun 2019 03:42:32 +0000 Gerrit-HasComments: Yes
