Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )
Change subject: [rest] add rest implementation ...................................................................... Patch Set 38: (16 comments) http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/CMakeLists.txt File src/kudu/rest/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/CMakeLists.txt@23 PS38, Line 23: add_library(rest controller.h I believe headers don't need to be added here. http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h File src/kudu/rest/controller.h: http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h@77 PS38, Line 77: info->addResponse<String>(Status::CODE_400, "text/plain"); shouldn't error handling also be JSON? does Oat++ support that? http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h@85 PS38, Line 85: info->addResponse<String>(Status::CODE_200, "text/plain"); why are the success responses here and below text/plain? Also, maybe we could use status code 201 here and 204 below if there's no content? http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h@88 PS38, Line 88: CreateKuduTable nit: missing space after comma here and below http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h@97 PS38, Line 97: ENDPOINT("PUT","/AlterKuduTableName",AlterKuduTableName, maybe it should be called RenameTable? http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.h@114 PS38, Line 114: client::sp::shared_ptr<client::KuduClient>* client); nit: alignment is off here and below http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc File src/kudu/rest/controller.cc: http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@68 PS38, Line 68: Controller::Controller(const std::shared_ptr<oatpp::parser::json::mapping::ObjectMapper>& object_mapper, nit: this and some other lines are too long - it's okay to break line after "<" for example http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@143 PS38, Line 143: string name; Why do these variables exist? Can't the DTO variables be set directly? http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@153 PS38, Line 153: name = kudu_schema.Column(i).name(); maybe &kudu_schema.Column(i) can be assigned to a local variable to avoid having to call Column() multiple times http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@176 PS38, Line 176: .default_admin_operation_timeout(MonoDelta::FromSeconds(20)) why is this specified here? http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@184 PS38, Line 184: KuduTableCreator* table_creator = client->NewTableCreator(); you could use a smart pointer to avoid having to manually delete it, then you can simply return Create(). http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@198 PS38, Line 198: string name; same as above http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/controller.cc@213 PS38, Line 213: if (isPrimaryKey) { you can simplify this by doing auto c = b.AddColumn(kudu_column_dto->columnName->std_str()), then if (kudu_column_dto->isPrimaryKey) c->PrimaryKey() etc. http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/rest_server.cc File src/kudu/rest/rest_server.cc: http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/rest_server.cc@72 PS38, Line 72: .setVersion("1.0") I think it should use the Kudu version instead of a hardcoded 1.0. http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/rest_server.cc@106 PS38, Line 106: ParseString a better function name would be ParseBindAddress, or something that tells more about what this function does. http://gerrit.cloudera.org:8080/#/c/17555/38/src/kudu/rest/rest_server.cc@107 PS38, Line 107: std::pair<string, string> p = strings::Split(rest_bind_address, strings::delimiter::Limit(":", 1)); nit: line too long -- 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: 38 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 19 Jul 2021 08:01:02 +0000 Gerrit-HasComments: Yes
