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/06/30 01:14:13 UTC
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16124
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes authorization checks
(though I left TODOs indicating that only the service user should be
able to create this table) and HMS synchronization.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
18 files changed, 430 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/1
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 9: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16124/9/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/16124/9/src/kudu/integration-tests/txn_status_table-itest.cc@162
PS9, Line 162: // While we prevent the table from being listed, clients should still be able
: // to view it if it's explicitly asked for.
Thank you for adding these.
BTW, is that the behavior expected to be there long-term or you have plans to hide those tables from TableExists() and OpenTable() in the future?
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h
File src/kudu/transactions/txn_system_client.h:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h@57
PS8, Line 57: (awong): when we imple
> It will likely live on the masters, since it seems easy enough to communica
Thanks for the clarification: yep, this sounds good to me.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 24 Jul 2020 05:26:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16124/9/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/16124/9/src/kudu/integration-tests/txn_status_table-itest.cc@162
PS9, Line 162: // While we prevent the table from being listed, clients should still be able
: // to view it if it's explicitly asked for.
> I don't have strong opinions about this, so it'll probably be the long-term
Thank you for the clarification. This sounds good for me.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 25 Jul 2020 01:56:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Reviewed-on: http://gerrit.cloudera.org:8080/16124
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 673 insertions(+), 50 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 20 Jul 2020 17:53:47 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#4).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes authorization checks
(though I left TODOs indicating that only the service user should be
able to create this table) and HMS synchronization.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
18 files changed, 441 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/4
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#7).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 640 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/7
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16124/9/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/16124/9/src/kudu/integration-tests/txn_status_table-itest.cc@162
PS9, Line 162: // While we prevent the table from being listed, clients should still be able
: // to view it if it's explicitly asked for.
> Thank you for adding these.
I don't have strong opinions about this, so it'll probably be the long-term behavior. I don't see the harm in keeping it if a user explicitly asks for it.
It'd be particularly surprising if a user tries to take the table name but it already exists, and then the user tried to call TableExists() and found that it didn't exist.
I can imagine in the future we might also want a ListTable() variant that includes system tables, e.g. for health checks and checking skew and such.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 24 Jul 2020 07:07:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#10).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 661 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/10
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/master_authz-itest.cc@685
PS8, Line 685: ient_);
> Does it make sense to add a scenario when such a call is performed using ot
Done
http://gerrit.cloudera.org:8080/#/c/16124/8/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/16124/8/src/kudu/integration-tests/txn_status_table-itest.cc@152
PS8, Line 152: table
> What about KuduClient::TableExists() and KuduClient::GetTableSchema()? Doe
Done
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/txn_status_table-itest.cc@279
PS8, Line 279:
> nit: creation of ?
Done
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h
File src/kudu/transactions/txn_system_client.h:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h@43
PS8, Line 43: // calls to various servers.
: class TxnSystemClient {
: public:
: static Status Create(const std::vector<std::string>& master_addrs,
: std::unique_ptr<TxnSystemClient>* sys_client);
:
> Do you mind adding a comment on the idea behind exposing these methods as p
I made it public to make it easier to interchange KuduClients. I'll make this private and add a friend class instead though.
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h@57
PS8, Line 57: (awong): when we imple
> What about dropping transaction status table range? What component is goin
It will likely live on the masters, since it seems easy enough to communicate most context to them in terms of what transaction ID ranges may be fully quiesced, e.g. via tserver heartbeating.
For now, I'm leaving it as a TODO because I don't see it as a necessity to get the basic orchestration functionality up and running, and I'd like to get that together ASAP so more people can begin working in parallel (e.g. once orchestration is in place, we can begin thinking more concretely about how Impala will interact with us).
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.cc@80
PS8, Line 80: 1
> Is it really intended to be a non-replicated table?
For now, yes, as per the above TODO.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 23 Jul 2020 23:50:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG@14
PS4, Line 14: kudu_system
> Should we enforce that all non-user tables are under this namespace?
Yes, but maybe when we add a second non-user table.
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@383
PS4, Line 383: using std::pair;
> warning: using decl 'map' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1531
PS4, Line 1531: // Do some fix-up of any defaults specified on columns.
> Would we still want this for system tables? Then the owner would be the kud
Hrm, good point. The system client would be a part of the server and that should mean that the table's owner would be the service user.
Re: requiring ownership on table access, I just leveraged the default authorization for the admin service, from tserver_admin.proto
service TabletServerAdminService {
option (kudu.rpc.default_authz_method) = "AuthorizeServiceUser";
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1540
PS4, Line 1540:
> Why don't system tables need this?
I forwent these schema checks because we're passing in a statically-defined schema that we know is correct.
That said, who's to say the statically-defined schema doesn't eventually change and break things. I'll move this out of this block.
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1559
PS4, Line 1559: return SetupError(
> Are you planning to do this in a follow on commit? Would it be easy enough to enforce requiring a superuser temporarily as opposed to being completely open?
That's always the case with TODOs :) As it pertains to the this feature, it would have been a must-do before calling the feature done for sure, given it would have been an easy attack vector.
[post-update] The change was easy, but even a seemingly a simple change can have pretty heavy implications for testing, etc.
In any case, good catch. Updated so we never list the system tables, even if we own them.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 16 Jul 2020 06:05:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#5).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 639 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/5
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#6).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 640 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/6
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#8).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 640 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/8
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/master_authz-itest.cc@685
PS8, Line 685: kAdminUser
Does it make sense to add a scenario when such a call is performed using other than admin user's creds?
http://gerrit.cloudera.org:8080/#/c/16124/8/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/16124/8/src/kudu/integration-tests/txn_status_table-itest.cc@152
PS8, Line 152: table
What about KuduClient::TableExists() and KuduClient::GetTableSchema()? Does it make sense to cover those methods as well?
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/integration-tests/txn_status_table-itest.cc@279
PS8, Line 279: creation
nit: creation of ?
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h
File src/kudu/transactions/txn_system_client.h:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h@43
PS8, Line 43: static Status Create(const std::vector<std::string>& master_addrs,
: std::unique_ptr<TxnSystemClient>* sys_client);
: static Status CreateTxnStatusTableWithClient(int64_t initial_upper_bound,
: client::KuduClient* client);
: static Status AddTxnStatusTableRangeWithClient(int64_t lower_bound, int64_t upper_bound,
: client::KuduClient* client);
Do you mind adding a comment on the idea behind exposing these methods as public ones given there are non-static counterparts as well?
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.h@57
PS8, Line 57: AddTxnStatusTableRange
What about dropping transaction status table range? What component is going to provide such a functionality?
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.cc
File src/kudu/transactions/txn_system_client.cc:
http://gerrit.cloudera.org:8080/#/c/16124/8/src/kudu/transactions/txn_system_client.cc@80
PS8, Line 80: 1
Is it really intended to be a non-replicated table?
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 23 Jul 2020 05:43:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#11).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 673 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/11
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#9).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes fine-grained
authorization checks and HMS synchronization.
Coarse-grained access to this table is table is granted only to the
service- or super-user. For the sake of the create-table and
add-partition functionality (which will likely come from the Kudu
service rather than a user), the CreateTable and AlterTable master RPC
endpoints now permit access to the service user.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
20 files changed, 673 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/9
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16124
to look at the new patch set (#3).
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
KUDU-2612 p4: mechanism to create a transaction status table
This plumbs table type into the master, and exercises the code paths by
adding a TxnSystemClient that can set the table type when creating
tables (as opposed to KuduClient, which cannot send such requests).
This patch adds the ability to create a transaction status table named
"kudu_system.kudu_transactions", which foregoes authorization checks
(though I left TODOs indicating that only the service user should be
able to create this table) and HMS synchronization.
This TxnSystemClient can be used by leader masters in the future to
create the initial transaction status table, but for now is useful for
testing the plumbing of table type.
Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_creator-internal.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
A src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/transactions/CMakeLists.txt
M src/kudu/transactions/txn_status_tablet.cc
M src/kudu/transactions/txn_status_tablet.h
A src/kudu/transactions/txn_system_client.cc
A src/kudu/transactions/txn_system_client.h
M src/kudu/tserver/tablet_service.cc
18 files changed, 430 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/3
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16124 )
Change subject: KUDU-2612 p4: mechanism to create a transaction status table
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16124/4//COMMIT_MSG@14
PS4, Line 14: kudu_system
Should we enforce that all non-user tables are under this namespace?
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1531
PS4, Line 1531: if (user && !req.has_owner()) {
Would we still want this for system tables? Then the owner would be the kudu service user right?
If we did that, we could require ownership as a way of enforcing only Kudu can write/modify the table.
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1540
PS4, Line 1540: for (int i = 0; i < req.schema().columns_size(); i++) {
Why don't system tables need this?
http://gerrit.cloudera.org:8080/#/c/16124/4/src/kudu/master/catalog_manager.cc@1559
PS4, Line 1559: // TODO(awong): only allow the service user to do this.
Are you planning to do this in a follow on commit? Would it be easy enough to enforce requiring a superuser temporarily as opposed to being completely open?
With this commit system tables will show up in list table commands and potentially attempted to be backed up and restored with backup jobs.
--
To view, visit http://gerrit.cloudera.org:8080/16124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa
Gerrit-Change-Number: 16124
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 15 Jul 2020 14:39:02 +0000
Gerrit-HasComments: Yes