Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )
Change subject: [rest] add rest implementation ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/CMakeLists.txt File src/kudu/rest/CMakeLists.txt: PS9: nit: add license header and remove extra trailing new lines 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@22 PS9, Line 22: #include "oatpp/web/server/api/ApiController.hpp" Shouldn't these be brackets instead of double quotes? Also, the order and grouping of the includes is weird. This applies to other files as well. http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/controller.h@45 PS9, Line 45: ENDPOINT_INFO(kuduTableSchema) { do we need these in the header? can they be moved into a cc file? http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/controller.h@135 PS9, Line 135: oatpp::data::mapping::type::Vector<Object < ColumnDTO>> GetKuduColumnsAsOatppVector( can the definitions moved into a .cc file? http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/dto/alter_table_DTO.h File src/kudu/rest/dto/alter_table_DTO.h: PS9: nit: "dto" in the file names should be lower case here and in the other files. http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/dto/table_schema_DTO.h File src/kudu/rest/dto/table_schema_DTO.h: http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/dto/table_schema_DTO.h@29 PS9, Line 29: missing CODEGEN_END? http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/rest_server.h File src/kudu/rest/rest_server.h: http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/rest_server.h@33 PS9, Line 33: std::shared_ptr<Controller> controller_; do these all need to be pointers? Can't they be stack-allocated? If not, do they need to be shared? http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/rest_server.cc File src/kudu/rest/rest_server.cc: http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/rest_server.cc@27 PS9, Line 27: oatpp::String rest_url_oatpp = rest_url.data(); in cc files you can "using" these classes instead of writing the fully qualified names everywhere, especially if there's multiple occurrences. http://gerrit.cloudera.org:8080/#/c/17555/9/src/kudu/rest/rest_server.cc@36 PS9, Line 36: objectMapper_ = oatpp::parser::json::mapping::ObjectMapper::createShared(serializeConfig, deserializeConfig); nit: line too long here and in several other places below -- 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: 9 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 14 Jun 2021 12:27:58 +0000 Gerrit-HasComments: Yes
