[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Adar Dembo 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: Verified+1 Code-Review+2 (1 comment) Test failure was unrelated. 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. Yeah looks nice! -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Wed, 12 Jun 2019 03:43:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13573 ) Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Reviewed-on: http://gerrit.cloudera.org:8080/13573 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 194 insertions(+), 30 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- 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: merged Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 6 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13573 ) Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- 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: deleteReviewer Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Wed, 12 Jun 2019 03:42:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13573 to look at the new patch set (#5). Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 194 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/13573/5 -- 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: newpatchset Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Adar Dembo 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 4: (2 comments) 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. 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: shared_ptr client 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). -- 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: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Tue, 11 Jun 2019 18:35:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/13573/2/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/13573/2/src/kudu/client/client-internal.h@148 PS2, Line 148: /// Get the table information identified by 'table_identifier'. : /// : /// @param [in] client > Can you update this? Done http://gerrit.cloudera.org:8080/#/c/13573/2/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/2/src/kudu/integration-tests/tablet_history_gc-itest.cc@133 PS2, Line 133: return scanner.Open(); : }; : : // When the tablet history max age is set to 0, it's not possible to do a : // snapshot scan without a t > Since this is repeated below, could you encapsulate it into a lambda that r Done -- 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: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Tue, 11 Jun 2019 07:16:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13573 to look at the new patch set (#4). Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 206 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/13573/4 -- 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: newpatchset Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 4 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13573 to look at the new patch set (#3). Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 206 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/13573/3 -- 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: newpatchset Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 3 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Adar Dembo 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 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/13573/2/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/13573/2/src/kudu/client/client-internal.h@148 PS2, Line 148: // Retrieve table information about the table identified by 'table_id_or_name'. : // 'table_id_or_name' should contain a table id or name as 'identifier_type' : // is ID or NAME, respectively. Can you update this? http://gerrit.cloudera.org:8080/#/c/13573/2/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/2/src/kudu/integration-tests/tablet_history_gc-itest.cc@133 PS2, Line 133: shared_ptr table; : ASSERT_OK(client_->OpenTable(TestWorkload::kDefaultTableName, &table)); : KuduScanner scanner(table.get()); : ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT)); : Status s = scanner.Open(); Since this is repeated below, could you encapsulate it into a lambda that returns a Status? -- 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: 2 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 10 Jun 2019 16:39:24 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13573 to look at the new patch set (#2). Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 187 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/13573/2 -- 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: newpatchset Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 2 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2514 Part 2: Supports setting history max age sec for the specified table
Yao Xu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13573 Change subject: KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table .. KUDU-2514 Part 2: Supports setting history_max_age_sec for the specified table In the previous patch, we implemented the framework of extra configuration properties that are customizable on a per-table basis. So, we can add custom configurations to the specified table. In this patch, I also added the implementation of the C++ API. Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tablet/tablet.cc 12 files changed, 186 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/13573/1 -- 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: newchange Gerrit-Change-Id: Iaac649154ae3c60785736ab246074c4dcb5b8987 Gerrit-Change-Number: 13573 Gerrit-PatchSet: 1 Gerrit-Owner: Yao Xu