Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/17555 )
Change subject: [rest] add rest implementation ...................................................................... Patch Set 50: (12 comments) http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/integration-tests/rest_server-itest.cc File src/kudu/integration-tests/rest_server-itest.cc: http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/integration-tests/rest_server-itest.cc@84 PS50, Line 84: vector<string> ParseResponse(const std::vector<std::string>& response) { > We should probably try to use JSON response as an object and not convert it Yes, Oat++ has its own serialised/deserializer, it works with Data Transfer Objects, converts them from/into JSON. This function is just for testing purposes, to see if the response we've gotten matches the response that we were expecting. I've tested the consistency mostly manually at the moment. http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/integration-tests/rest_server-itest.cc@132 PS50, Line 132: vector<string> unparsed_response = strings::Split(response.ToString(),",", > nit: there are many places after this line where the spaces are missing aft Done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/master/master.cc@102 PS50, Line 102: DEFINE_bool(rest_server_enabled,false,"Should a rest endpoint be enabled on this master"); > nit: multiple lines are missing spaces after commas done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.h File src/kudu/rest/controller.h: http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.h@89 PS50, Line 89: Alter a Kudu table name > why not stick with "Rename a Kudu table"? Wanted to put emphasis that its only for altering the name of the table. Changed to Rename a Kudu Table http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc File src/kudu/rest/controller.cc: http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@67 PS50, Line 67: > nit: extra empty lines Done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@85 PS50, Line 85: Couldn't find a table with the provided name > maybe add the table name to the error message for user friendliness Done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@114 PS50, Line 114: status_dto->message = "Table either exists or client side error"; > what kind of client side error would trigger this? If the table with the same name exists, the client gets overloaded and times out before finalising the request or incorrect name for a table(aka forbidden characters) http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@137 PS50, Line 137: "Unsuccessful, either no table with such name exists or a client side error"; > what kind of client side error would trigger this? the client gets overloaded and times out before finalising the request or incorrect name for a table(aka forbidden characters) http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@192 PS50, Line 192: MonoDelta::FromSeconds(20) > how did you arrive to this timeout value? is there a default for this in th This should be the default value for timeout, I've taken it from the c++ kudu example http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@207 PS50, Line 207: delete table_creator; > you could use std::unique_ptr instead Done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/controller.cc@259 PS50, Line 259: delete table_alterer; > you could use std::unique_ptr instead Done http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/rest_server.cc File src/kudu/rest/rest_server.cc: http://gerrit.cloudera.org:8080/#/c/17555/50/src/kudu/rest/rest_server.cc@74 PS50, Line 74: .setVersion("1.15.0") > is this the version of the webserver or should this be the Kudu version? Yes, this should be the same as the Kudu Version, there is no way to update it dynamically though as it is only seen in the static Swagger component -- 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: 50 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: Wed, 25 May 2022 23:35:19 +0000 Gerrit-HasComments: Yes
