Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )
Change subject: [rest] add rest implementation ...................................................................... Patch Set 49: (16 comments) Also squash the commits together and updated the commit message to encapsulate all 3 commits in one http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/controller.h File src/kudu/rest/controller.h: http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/controller.h@45 PS9, Line 45: class ObjectMapper; > do we need these in the header? can they be moved into a cc file? The ENDPOINT_INFO is what the swagger uses to display the endpoints. http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/controller.h@135 PS9, Line 135: > can the definitions moved into a .cc file? Definitions can be moved into a .cc 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? Done 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 As there are several Object Mappers in Oat++, (one from ApiController class) unless if I don't specify it directly the compiler is unable to distinguish between them 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 Done 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 Working on this at the moment 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 Working on this 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? Working on this 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 I can use SCOPED_CLEANUP({delete table_alterer;}) but don't seem to be able to use the 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 Done 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 Done 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 Done 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? Clang recommended it to make it explicit, no particular reason other than that 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 Seeing as this method is used by this class only in case of constructing a new rest endpoint, I've kept it here, but I've switched it to be private and can move it to anonymous namespace http://gerrit.cloudera.org:8080/#/c/17555/49/src/kudu/rest/rest_server.h@71 PS49, Line 71: std::string *host, > nit: indent Done 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 Its in the build files, resources used to build the swagger page, defined inside the CMakeLists.txt of the Rest -- 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: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 19 May 2022 18:53:44 +0000 Gerrit-HasComments: Yes
