Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )
Change subject: [rest] add rest implementation ...................................................................... Patch Set 42: (6 comments) Not a full review, but should help with the build issue. http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/controller.cc File src/kudu/rest/controller.cc: http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/controller.cc@53 PS42, Line 53: namespace oatpp { : namespace web { : namespace protocol { : namespace http { : namespace outgoing { : class Response; : } : } // namespace http : } // namespace protocol : } // namespace web : } // namespace oatpp With C++17, we should be able to use namespace oatpp::web::protocol::http::outgoing { class Response; } // namespace oatpp::web::protocol::http::outgoing See http://www.nuonsoft.com/blog/2017/08/01/c17-nested-namespaces/ http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/controller.cc@77 PS42, Line 77: KUDU_CHECK_OK On failure, CHECKs crash the process. Instead, I think we ought to catch errors and return an appropriate StatusDTO. Something like: Status GetTableSchema(const String& table_name, KuduSchema* kudu_schema, bool* exists) { shared_ptr<KuduClient> kudu_client; RETURN_NOT_OK(CreateClient(master_addresses_, kudu_client)); auto kudu_table_schema_dto = TableSchemaDTO::createShared(); kudu_table_schema_dto->kuduColumns = {}; kudu_table_schema_dto->tableName = table_name; RETURN_NOT_OK(DoesTableExist(kudu_client, table_name->std_str(), &exists)); if (*exists) { RETURN_NOT_OK(kudu_client->GetTableSchema(table_name->std_str(), kudu_schema)); } return Status::OK(); } std::shared_ptr<Response> Controller::KuduTableSchema(const String& table_name) { bool exists; KuduSchema kudu_schema; Status s = GetTableSchema(table_name, &kudu_schema, &exists); if (!s.ok()) { // ... create an error status return dto response; } if (!exists) { auto status_dto = StatusDTO::createShared(); status_dto->code = 400; status_dto->status = ""; status_dto->message = "Couldn't find a table with the provided name"; return createDtoResponse(Status::CODE_400, status_dto); } kudu_table_schema_dto->kuduColumns = GetKuduColumnsAsOatppVector(*kudu_schema); return createDtoResponse(Status::CODE_200, kudu_table_schema_dto); } In addition to avoiding a crash, that way we could also encapsulate the table creation logic and separate it from the error-reporting logic, which would be a bit easier to reason about. http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/controller.cc@106 PS42, Line 106: KuduSchema schema = CreateSchema(kudu_table_schema_dto, part_col); > error: no viable conversion from 'shared_ptr<vector<std::__cxx11::string> > You're seeing this issue because the "using kudu::client::sp::shared_ptr" decl means a "shared_ptr" is going to be a kudu::client::sp::shared_ptr. Instead, you should change L104 to be "std::shared_ptr". That said, why does part_col need to be a shared_ptr at all? Why not just a vector<string>? http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/rest_server.cc File src/kudu/rest/rest_server.cc: http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/rest_server.cc@26 PS42, Line 26: #include <oatpp/network/Server.hpp> : #include <oatpp/network/tcp/server/ConnectionProvider.hpp> : #include <oatpp/web/server/HttpConnectionHandler.hpp> : #include <oatpp/web/server/HttpRouter.hpp> : #include <oatpp-swagger/Controller.hpp> : #include <oatpp-swagger/Resources.hpp> nit: move the thirdparty headers so it's in between the system headers and Kudu headers. http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/rest_server.cc@61 PS42, Line 61: serializeConfig nit: convert camelCase to snake_case in this file http://gerrit.cloudera.org:8080/#/c/17555/42/src/kudu/rest/rest_server.cc@116 PS42, Line 116: port=8061; : } else if (SimpleAtoi(p.second, &port_num)) { : port=port_num; nit: fix the spacing -- 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: 42 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-Comment-Date: Tue, 03 Aug 2021 19:51:45 +0000 Gerrit-HasComments: Yes
