Zoltan Chovan 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 into a string vector. What ensures that the order of fields present is consistently the same? Seeing that EasyJSON is for building Json objects, does Oat++ have any JSON serializer/deserializer functionality? 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 after commas (, ) 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 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"? 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 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 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? 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? 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 the Kudu client? 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 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 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? -- 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 13:12:19 +0000 Gerrit-HasComments: Yes
