You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/05/11 18:45:45 UTC
[kudu-CR] catalog-manager: split AlterTable and DeleteTable
Hello Adar Dembo, Hao Hao,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10378
to review the following change.
Change subject: catalog-manager: split AlterTable and DeleteTable
......................................................................
catalog-manager: split AlterTable and DeleteTable
These methods will work quite differently once full HMS support lands,
and it will simplify them to be split into the sections that modify the
Kudu sys catalog, and the sections which only deal with HMS.
Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
4 files changed, 62 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/10378/1
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 14 May 2018 18:01:45 +0000
Gerrit-HasComments: No
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10378
to look at the new patch set (#2).
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
catalog-manager: refactor AlterTable and DeleteTable methods
This commit splits CatalogManager::AlterTable and
CatalogManager::DeleteTable in two. One method handles RPC specifics,
and the second handles applying the alter/delete operation to the Kudu
catalog. The RPC-handling method thus calls into the method which
modifies the catalog. A follow-up commit in the HMS integration series
will add another front-end method specific to HMS notification log
listener events.
Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
4 files changed, 61 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/10378/2
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10378/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10378/1//COMMIT_MSG@7
PS1, Line 7: refactor AlterTable and DeleteTa
> FWIW I found this confusing at first, because I thought it meant that Alter
Done
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 14 May 2018 17:51:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog-manager: split AlterTable and DeleteTable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: split AlterTable and DeleteTable
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10378/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10378/1//COMMIT_MSG@7
PS1, Line 7: split AlterTable and DeleteTable
FWIW I found this confusing at first, because I thought it meant that AlterTable and DeleteTable were implemented in the same method, but now you're splitting them up. Only after I looked at the patch did I realize what you meant.
Maybe reword to clarify?
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 11 May 2018 19:04:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10378
to look at the new patch set (#3).
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
catalog-manager: refactor AlterTable and DeleteTable methods
This commit splits CatalogManager::AlterTable and
CatalogManager::DeleteTable in two. One method handles RPC specifics,
and the second handles applying the alter/delete operation to the Kudu
catalog. The RPC-handling method thus calls into the method which
modifies the catalog. A follow-up commit in the HMS integration series
will add another front-end method specific to HMS notification log
listener events.
Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
4 files changed, 62 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/10378/3
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
catalog-manager: refactor AlterTable and DeleteTable methods
This commit splits CatalogManager::AlterTable and
CatalogManager::DeleteTable in two. One method handles RPC specifics,
and the second handles applying the alter/delete operation to the Kudu
catalog. The RPC-handling method thus calls into the method which
modifies the catalog. A follow-up commit in the HMS integration series
will add another front-end method specific to HMS notification log
listener events.
Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Reviewed-on: http://gerrit.cloudera.org:8080/10378
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master_service.cc
4 files changed, 62 insertions(+), 44 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
Patch Set 3:
(1 comment)
> Patch Set 2:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/10378/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10378/2/src/kudu/master/catalog_manager.cc@2143
PS2, Line 2143: RETURN_NOT_OK(SetupError(
> error: member reference type 'const kudu::master::AlterTableRequestPB' is n
Done
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 14 May 2018 18:46:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] catalog-manager: refactor AlterTable and DeleteTable methods
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: refactor AlterTable and DeleteTable methods
......................................................................
Patch Set 3: Code-Review+2
Carrying over Adar's +2
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 14 May 2018 19:19:31 +0000
Gerrit-HasComments: No
[kudu-CR] catalog-manager: split AlterTable and DeleteTable
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10378 )
Change subject: catalog-manager: split AlterTable and DeleteTable
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia384768ee7246411052ccadc66c33e83b541c195
Gerrit-Change-Number: 10378
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 11 May 2018 22:21:37 +0000
Gerrit-HasComments: No