[kudu-CR] [rest] add rest implementation

["Attila Bukor (Code Review)" <ge...@cloudera.org>]

Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )

Change subject: [rest] add rest implementation
......................................................................


Patch Set 49:

(14 comments)

As this patch doesn't have tests, I wonder if it would make sense to squash the follow-up patches in the chain to this commit? They're not particularly big and kind of belong together.

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc
File src/kudu/rest/controller.cc:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@54
PS49, Line 54: class Response;
shouldn't this be included instead?


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@61
PS49, Line 61:     const std::shared_ptr<oatpp::parser::json::mapping::ObjectMapper>& object_mapper,
nit: you can "using" these in .cc files


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@63
PS49, Line 63:     : ApiController(object_mapper), master_addresses_(ParseString(master_addresses)) {
nit: master_addresses_ should be in a new line


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@69
PS49, Line 69: std::shared_ptr<Response> Controller::KuduTableSchema(const String& table_name) {
how about refactoring the logic into separate methods that return a Status and that way you can use the controller methods to just return a response based on the Status? That way you can use RETURN_NOT_OK() instead of CHECK() calls (which could crash the server).


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@207
PS49, Line 207:   delete table_creator;
you can use SCOPED_CLEANUP and RETURN_NOT_OK


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@242
PS49, Line 242:   kudu::Status s = client->OpenTable(table_name, &table);
maybe you could use RETURN_NOT_OK_EVAL?


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/controller.cc@259
PS49, Line 259:   delete table_alterer;
you can use SCOPED_CLEANUP and RETURN_NOT_OK


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/alter_table_dto.h
File src/kudu/rest/dto/alter_table_dto.h:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/alter_table_dto.h@25
PS49, Line 25: #include OATPP_CODEGEN_BEGIN(DTO)
nit: add a newline here and below


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/column_dto.h
File src/kudu/rest/dto/column_dto.h:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/column_dto.h@25
PS49, Line 25: #include OATPP_CODEGEN_BEGIN(DTO)
nit: add a newline here and below


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/table_schema_dto.h
File src/kudu/rest/dto/table_schema_dto.h:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/dto/table_schema_dto.h@26
PS49, Line 26: #include OATPP_CODEGEN_BEGIN(DTO)
nit: add a newline here and below


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.h
File src/kudu/rest/rest_server.h:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.h@68
PS49, Line 68:   explicit RestServer(const std::string& master_addresses, const std::string& rest_bind_address);
why is this explicit?


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.h@70
PS49, Line 70:   static void ParseBindAddress(const std::string& rest_bind_address,
Does this need to be a public function? Shouldn't this be in the anonymous namespace instead defined in .cc?


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.h@71
PS49, Line 71:                         std::string *host,
nit: indent


http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.cc
File src/kudu/rest/rest_server.cc:

http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.cc@70
PS49, Line 70:   auto swaggerResources = Resources::loadResources(OATPP_SWAGGER_RES_PATH);
where is this OATPP_SWAGGER_RES_PATH coming from? IWYU is complaining about this one too.



-- 
To view, visit http://gerrit.cloudera.org:8080/17555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2ca3121fd7e95a1267853be45cb5f5855298c763
Gerrit-Change-Number: 17555
Gerrit-PatchSet: 49
Gerrit-Owner: Khazar Mammadli <ma...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 May 2022 14:21:41 +0000
Gerrit-HasComments: Yes