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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 16 May 2022 14:21:41 +0000 Gerrit-HasComments: Yes
