You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/10/07 21:56:30 UTC
[kudu-CR] tserver: only list tablets of user-defined tables by default
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/16560
to review the following change.
Change subject: tserver: only list tablets of user-defined tables by default
......................................................................
tserver: only list tablets of user-defined tables by default
Several tests depend on the fact that a cluster starts up with no
tablets. This will become a problem if we start creating the transaction
status table upon startup. So, in an effort to prevent such tests from
failing, this patch avoids listing system tables by default.
The optionality of the table_type() interface in TabletMetadata felt
needlessly clunky in implementing this, so I made the field non-optional
instead, opting to explicilty use DEFAULT_TABLE where appropriate. I
left the public constructors alone to avoid requiring common.pb.h in
every test that wanted to create a DEFAULT_TABLE tablet.
Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
---
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
9 files changed, 88 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/16560/1
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] tserver: only list tablets of user-defined tables by default
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: tserver: only list tablets of user-defined tables by default
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16560/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16560/1//COMMIT_MSG@16
PS1, Line 16: explicilty
explicitly
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/integration-tests/txn_status_table-itest.cc
File src/kudu/integration-tests/txn_status_table-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/integration-tests/txn_status_table-itest.cc@208
PS1, Line 208: TEST_F(TxnStatusTableITest, TestDontListTablets) {
I'm not sure I see the coverage for the following things:
* specifying TableTypePB::DEFAULT_TABLE for ListTabletsRequestPB::type_filter gives the same result as if not specifying it at all for _both_ when user tables are present and abset
* how the filter works in the presence and absence of user and system tables
Does it make sense to add coverage for that as well?
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/mini_tablet_server.cc@158
PS1, Line 158: vector<string> MiniTabletServer::ListTablets(set<TableTypePB> type_filter) const {
> warning: the parameter 'type_filter' is copied for each invocation but only
+1
Consider passing the 'type_filter' parameter by const reference?
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/tablet_service.cc@2103
PS1, Line 2103: set<TableTypePB> types_to_return = { TableTypePB::DEFAULT_TABLE };
What if request doesn't list DEFAULT_TABLE in its 'filter_type' field?
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 07 Oct 2020 22:33:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
[tserver] introduce filter_type for ListTablets
This patch adds 'filter_type' field into ListTabletsRequestPB. If
the field is not set, it's interpreted as if the field were set to
[DEFAULT_TABLE]. That's to return only user tables by default even
if the transaction status table is present.
By introducing such a behavior, this patch addresses the failure
of 50+ tests when a transaction status table is created by TxnManager
upon first startup of Kudu master (see the upcoming patch at
https://gerrit.cloudera.org/#/c/16527). Those tests depend on the
fact that a cluster starts up with no tablets.
The optionality of the table_type() interface in TabletMetadata felt
needlessly clunky in implementing this, so I made the field non-optional
instead, opting to explicitly use DEFAULT_TABLE where appropriate. I
left the public constructors alone to avoid requiring common.pb.h in
every test that wanted to create a DEFAULT_TABLE tablet.
Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
---
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver.proto
10 files changed, 181 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/16560/2
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#3) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
[tserver] introduce filter_type for ListTablets
This patch adds 'filter_type' field into ListTabletsRequestPB. If
the field is not set, it's interpreted as if the field were set to
[DEFAULT_TABLE]. That's to return only user tables by default even
if the transaction status table is present.
By introducing such a behavior, this patch addresses the failure
of 50+ tests when a transaction status table is created by TxnManager
upon first startup of Kudu master (see the upcoming patch at
https://gerrit.cloudera.org/#/c/16527). Those tests depend on the
fact that a cluster starts up with no tablets.
The optionality of the table_type() interface in TabletMetadata felt
needlessly clunky in implementing this, so I made the field non-optional
instead, opting to explicitly use DEFAULT_TABLE where appropriate. I
left the public constructors alone to avoid requiring common.pb.h in
every test that wanted to create a DEFAULT_TABLE tablet.
Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
---
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/mini_tablet_server.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver.proto
10 files changed, 183 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/16560/3
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
Patch Set 3:
> We chatted about this a bit. Given the new lazy creation of the
> transaction status table, seems this is no long required to unblock
> tests.
>
> We may want this filtering in the future, given not all users may
> want to see system tables. That said, it isn't obvious how
> prominent this filtering should be (e.g. should we also filter the
> web UI? should we update all tooling?). For now, let's let this sit
> and circle back if any strong arguments for continuing this work
> come up.
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 14 Oct 2020 23:09:59 +0000
Gerrit-HasComments: No
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
Patch Set 3:
We chatted about this a bit. Given the new lazy creation of the transaction status table, seems this is no long required to unblock tests.
We may want this filtering in the future, given not all users may want to see system tables. That said, it isn't obvious how prominent this filtering should be (e.g. should we also filter the web UI? should we update all tooling?). For now, let's let this sit and circle back if any strong arguments for continuing this work come up.
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 14 Oct 2020 18:10:50 +0000
Gerrit-HasComments: No
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16560/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16560/1//COMMIT_MSG@16
PS1, Line 16: of Kudu ma
> explicitly
Done
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/mini_tablet_server.cc
File src/kudu/tserver/mini_tablet_server.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/mini_tablet_server.cc@158
PS1, Line 158: vector<string> MiniTabletServer::ListTablets(
> warning: the parameter 'type_filter' is copied for each invocation but only
Done
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/mini_tablet_server.cc@158
PS1, Line 158: vector<string> MiniTabletServer::ListTablets(
> +1
Done
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/tserver/tablet_service.cc@2103
PS1, Line 2103:
> What if request doesn't list DEFAULT_TABLE in its 'filter_type' field?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 08 Oct 2020 05:40:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
Abandoned
As mentioned, not needed at the moment. May revisit in the future.
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [tserver] introduce filter type for ListTablets
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16560 )
Change subject: [tserver] introduce filter_type for ListTablets
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/integration-tests/txn_status_table-itest.cc
File src/kudu/integration-tests/txn_status_table-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16560/1/src/kudu/integration-tests/txn_status_table-itest.cc@208
PS1, Line 208: TEST_F(TxnStatusTableITest, TestDontListTablets) {
> I'm not sure I see the coverage for the following things:
Done
--
To view, visit http://gerrit.cloudera.org:8080/16560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f8d34a8c377fcdef0162eaf042498a6d41baa4a
Gerrit-Change-Number: 16560
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 08 Oct 2020 05:40:55 +0000
Gerrit-HasComments: Yes