You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Gabriella Lotz (Code Review)" <ge...@cloudera.org> on 2025/01/29 14:27:34 UTC
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Gabriella Lotz has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22420
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
[KUDU-3639] Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 346 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/1
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/983/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 29 Jan 2025 14:27:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Zoltan Martonka, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#4).
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 306 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/4
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 4:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1012/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 13:36:26 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3: -Verified
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1011/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 12:31:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Zoltan Martonka, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#6).
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 303 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/6
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2025 14:09:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager-test.cc
File src/kudu/master/catalog_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager-test.cc@339
PS1, Line 339: req.mutable_table()->set_table_name("test_table");
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: ASSERT_OK(master_->catalog_manager()->DeleteTableRpc(req, &resp, nullptr));
: }
:
: TEST_F(CatalogManagerTest, TestDeleteTableWithUser) {
: CreateTestTable();
: DeleteTableRequestPB req;
: DeleteTableResponsePB resp;
: req.mutable_table()->set_table_name("test_table");
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: const string user = "test_user";
: ASSERT_OK(master_->catalog_manager()->DeleteTableWithUser(req, &resp, user));
: }
:
: TEST_F(CatalogManagerTest, TestCreateTableRpc) {
: CreateTableRequestPB req;
: CreateTableResponsePB resp;
: CreateTableTest(req, resp);
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: ASSERT_OK(master_->catalog_manager()->CreateTable(&req, &resp, nullptr));
: }
:
: TEST_F(CatalogManagerTest, TestCreateTableWithUser) {
: CreateTableRequestPB req;
: CreateTableResponsePB resp;
: CreateTableTest(req, resp);
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: const string user = "test_user";
: ASSERT_OK(master_->catalog_manager()->CreateTableWithUser(&req, &resp, user));
: }
:
: TEST_F(CatalogManagerTest, TestAlterTableRpc) {
: CreateTestTable();
: AlterTableRequestPB req;
:
> Please make a shared function.
Done
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2025 09:45:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 8:
> Thank you for the comments! I have created a follow-up patch for
> this one: https://gerrit.cloudera.org/c/22529/
Thanks a lot for addressing these comments!
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Feb 2025 02:25:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/988/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2025 10:20:57 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 5:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1020/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 5
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2025 10:18:40 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has removed a vote on this change.
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2025 22:19:13 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/987/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 2
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jan 2025 16:28:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3: Verified+1
Build Successful
http://jenkins.kudu.apache.org/job/pre_commit/1011/ : SUCCESS
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 13:28:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/988/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2025 09:45:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 10:40:06 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 5: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1020/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 5
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Feb 2025 11:49:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 14:16:18 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#2).
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
[KUDU-3639] Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 347 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/2
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 2
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 8:
> Patch Set 8:
>
> (14 comments)
>
> Thank you for working on this!
>
> I wasn't in the list of reviewers, but I glanced over the updated code in my git workpace after pulling in the recent updates, so I decided to chime in even if this patch is already pushed.
Thank you for the comments! I have created a follow-up patch for this one: https://gerrit.cloudera.org/c/22529/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2025 11:47:45 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Zoltan Chovan, Zoltan Martonka, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#7).
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 298 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/7
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 6: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1031/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2025 13:13:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 6: Code-Review+1
(4 comments)
Just a couple of nits, and I think we are good.
Thank you for working on this!
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc
File src/kudu/master/catalog_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@277
PS6, Line 277: CatalogManagerTest
Please rename the test class to reflect whats the purpose of this test.
Something like this would do the job: CatalogManagerRpcAndUserFunctionsTest
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316
PS6, Line 316: CreateTableTest
Please rename this function as is can be confusing to have a CreateTestTable and a CreateTableTest function.
This function could be rename to something like: PopulateCreateTableRequest.
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316
PS6, Line 316: CreateTableResponsePB& resp
if not used in the function, you can remove this argument.
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc@2233
PS6, Line 2233: if (!rpc) {
: LOG(INFO) << "rpc is nullptr, calling CreateTableHelper with nullptr.";
: return CreateTableHelper(orig_req, resp, nullptr, std::nullopt);
: }
:
: return CreateTableHelper(orig_req, resp, rpc, std::nullopt);
nit:
i think we dont need the if check there, right?
we can just simply call:
return CreateTableHelper(orig_req, resp, rpc, std::nullopt);
In the helper function where rpc is used we have proper if checks there.
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 Feb 2025 18:56:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7: Verified+1
unrelated test failure: asan: maintenance_mode-itest.4
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 10:38:53 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1055/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 08:54:29 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/987/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 2
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jan 2025 17:51:09 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Zoltan Martonka, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#5).
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 304 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/5
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 5
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@1909
PS1, Line 1909: user.emplace(rpc.value()->remote_user().username());
> Is this log needed?
Done
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@1949
PS1, Line 1949: ship.
> Is it ok to wait forever here?
In WaitForNotificationLogListenerCatchUp, WaitForCatchUp is called with rpc->GetClientDeadline(). Looking at the GetClientDeadline() it says in the code: If the client did not specify a deadline, returns MonoTime::Max(). Since this is the default, I used it in case there isn't RPC context available.
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@3453
PS1, Line 3453: user.emplace(rpc.value()->remote_user().username());
> Is this important to log on INFO level?
Done
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Jan 2025 09:45:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Zoltan Martonka, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22420
to look at the new patch set (#3).
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 339 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/22420/3
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 3
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager-test.cc
File src/kudu/master/catalog_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager-test.cc@339
PS1, Line 339: TEST_F(CatalogManagerTest, TestCreateTableRpc) {
: CreateTableRequestPB req;
: CreateTableResponsePB resp;
: SchemaPB* schema = req.mutable_schema();
: ColumnSchemaPB* col = schema->add_columns();
: col->set_name("key");
: col->set_type(INT32);
: col->set_is_key(true);
: ColumnSchemaPB* col2 = schema->add_columns();
: col2->set_name("int_val");
: col2->set_type(INT32);
: req.set_name("test_table");
: req.set_owner("default");
: req.set_num_replicas(1);
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: ASSERT_OK(master_->catalog_manager()->CreateTable(&req, &resp, nullptr));
: }
:
: TEST_F(CatalogManagerTest, TestCreateTableWithUser) {
: CreateTableRequestPB req;
: CreateTableResponsePB resp;
: SchemaPB* schema = req.mutable_schema();
: ColumnSchemaPB* col = schema->add_columns();
: col->set_name("key");
: col->set_type(INT32);
: col->set_is_key(true);
: ColumnSchemaPB* col2 = schema->add_columns();
: col2->set_name("int_val");
: col2->set_type(INT32);
: req.set_name("test_table");
: req.set_owner("default");
: req.set_num_replicas(1);
: CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager());
: const string user = "test_user";
: ASSERT_OK(master_->catalog_manager()->CreateTableWithUser(&req, &resp, user));
: }
Please make a shared function.
This level of code duplication feels bad even in a test file.
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.h@1056
PS1, Line 1056: std::optional<rpc::RpcContext*> rpc,
Please leave a sort comment why do u need an optional pointer, when do we have + nullopt (New REST Api??)
+ nullptr (Current RPC API??)
+ a proper object (Current RPC??)
Nothing too serious, but an std::optional<something*> worth a comment.
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@1909
PS1, Line 1909: LOG(INFO) << "rpc exists but might be empty: " << rpc.value();
Is this log needed?
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@1949
PS1, Line 1949: MonoTime::Max()
Is it ok to wait forever here?
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.cc@3453
PS1, Line 3453: LOG(INFO) << "rpc value" << rpc.value();
Is this important to log on INFO level?
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jan 2025 17:18:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 6:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1031/
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 6
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Feb 2025 12:20:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
KUDU-3639 Add REST-compatible table operation functions
Added new functions to CatalogManager (CreateTableWithUser,
DeleteTableWithUser, and AlterTableWithUser) to support the REST API
implementation. These functions accept a username parameter instead of
relying on RpcContext, making them suitable for contexts where
RpcContext is unavailable.
Shared logic between the RPC-based and REST-compatible functions has
been refactored into reusable helper methods to minimize code
duplication.
Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Reviewed-on: http://gerrit.cloudera.org:8080/22420
Tested-by: Marton Greber <gr...@gmail.com>
Reviewed-by: Marton Greber <gr...@gmail.com>
Reviewed-by: Zoltan Martonka <zm...@cloudera.com>
---
M src/kudu/master/catalog_manager-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
3 files changed, 298 insertions(+), 47 deletions(-)
Approvals:
Marton Greber: Looks good to me, approved; Verified
Zoltan Martonka: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 8:
(14 comments)
Thank you for working on this!
I wasn't in the list of reviewers, but I glanced over the updated code in my git workpace after pulling in the recent updates, so I decided to chime in even if this patch is already pushed.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc
File src/kudu/master/catalog_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@297
PS8, Line 297: KUDU_CHECK_OK(
This should have been RETURN_NOT_OK instead.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@301
PS8, Line 301: KuduTableCreator* tableCreator
Wrap this into std::unique_ptr and remove 'delete' at line 312. It would be much less error prone that way.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@307
PS8, Line 307: KUDU_CHECK_OK
ditto
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@336
PS8, Line 336: CreateTestTable();
Here and below: Wrap this into ASSERT_OK()
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h@1060
PS8, Line 1060: std::optional<rpc::RpcContext*> rpc
Passing a pointer wrapped into std::optional doesn't make much sense unless it's necessary to distinguish between nullptr and the absence of a pointer at all. However, in the related code it's always checked for the presence of the pointer it's being a non-null. Also, creating std::optional objects is not free -- it takes some CPU cycles.
With that, why not to pass just a pointer here instead?
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h@1391
PS8, Line 1391: const std::optional<rpc::RpcContext*>& rpc_opt
This is a bit funny: in all the other places, the wrapper for the pointer is passed by value, but here it's passed by reference. Why is the difference?
If just a pointer was passed around, there would be no chance for ambiguity.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@a3362
PS8, Line 3362:
Maybe, it's better to leave it here but remove similar assert from AlterTableWithUser() and AlterTable() methods?
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2231
PS8, Line 2231: leader_lock_.AssertAcquiredForReading();
Why not to move this into CreateTableHelper() instead? That way it's less code and, most importantly, less bugs if anybody is about to start using CreateTableHelper() elsewhere.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2730
PS8, Line 2730: leader_lock_.AssertAcquiredForReading();
Why not to move this into DeleteTableHelper() instead?
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2737
PS8, Line 2737: from $0
Is there a way to provide the information on the source address where the request came from? That's valuable for auditing and troubleshooting purposes to have this in the logs.
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2739
PS8, Line 2739: leader_lock_.AssertAcquiredForReading();
Why not to move this into DeleteTableHelper() instead?
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@3501
PS8, Line 3501: leader_lock_.AssertAcquiredForReading();
Why not to move this into AlterTableHelper() instead?
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@3511
PS8, Line 3511: leader_lock_.AssertAcquiredForReading();
ditto
http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@6854
PS8, Line 6854: MonoTime::Max();
I don't think waiting insanely long makes any sense here. There should be some sane timeout interval. Maybe, introduce a new dedicated flag for this timeout with default value of, say, 30 seconds?
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 8
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2025 04:57:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc
File src/kudu/master/catalog_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@277
PS6, Line 277: CatalogManagerRpcA
> Please rename the test class to reflect whats the purpose of this test.
Done
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316
PS6, Line 316: c void Populate
> Please rename this function as is can be confusing to have a CreateTestTabl
Done
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316
PS6, Line 316: bleRequestPB* req) {
> if not used in the function, you can remove this argument.
Done
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc@2233
PS6, Line 2233: }
:
: // Create a new table.
: Status CatalogManager::CreateTableWithUser(const CreateTableRequestPB* orig_req,
: CreateTableResponsePB* resp,
: const optional<stri
> nit:
Done
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 08:54:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1055/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 10:05:52 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 7
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 10:38:57 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-3639] Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: [KUDU-3639] Add REST-compatible table operation functions
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/983/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jan 2025 14:37:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/22420/1/src/kudu/master/catalog_manager.h@1056
PS1, Line 1056: // authentication details, whereas CreateTableWithUser does not and instead relies on
> Please leave a sort comment why do u need an optional pointer, when do we h
Done
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 14:15:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3639 Add REST-compatible table operation functions
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions
......................................................................
Patch Set 4: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1012/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5
Gerrit-Change-Number: 22420
Gerrit-PatchSet: 4
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Feb 2025 14:53:43 +0000
Gerrit-HasComments: No