Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: [WIP] KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 17: (11 comments) http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@125 PS17, Line 125: if (table_id.length() != 32) { nit: add a few new lines (e.g. before if statements) for readability http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@136 PS17, Line 136: if (!status.ok()) { : jw.StartObject(); : jw.String("error"); : jw.String(status.ToString()); : jw.EndObject(); : resp->status_code = HttpStatusCode::ServiceUnavailable; : return; : } this kind of status check is present in almost every method introduced, extract the majority of these into a common function or macro 'RETURN_JSON_ERROR' or something like: #define RETURN_JSON_ERROR (error_msg, code) { JsonWriter jw(output, JsonWriter::COMPACT); jw.String("error"); \ jw.String(error_msg); \ jw.EndObject(); \ *status_code = code; return; } http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@145 PS17, Line 145: jw.StartObject(); could use macro/error handling function here as well, and all similar places across the methods http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@190 PS17, Line 190: Gabi probably better to use your github username for TODOs, to make it more specific for others http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@214 PS17, Line 214: } shouldn't the response status code be set to OK here? http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@237 PS17, Line 237: PrintTableObject(output, response.table_id()); shouldn't the response status code be set to OK here? http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@337 PS17, Line 337: *status_code = HttpStatusCode::NoContent; why NoContent status instead of OK? the delete was successful http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@135 PS17, Line 135: ASSERT_STR_CONTAINS(buf.ToString(), "{\"tables\":[]}"); probably should check the returned http status OK here http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@149 PS17, Line 149: } probably should check the returned http status OK here and everywhere else where we expect a successful outcome http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@165 PS17, Line 165: "{\"name\":\"test_table\",\"id\":\"$0\",\"schema\":{" : "\"columns\":[{\"id\":10,\"name\":\"key\",\"type\":\"INT32\",\"is_key\":true,\"is_" : "nullable\":false,\"encoding\":\"AUTO_ENCODING\",\"compression\":\"DEFAULT_COMPRESSION\"," : "\"cfile_block_size\":0,\"immutable\":false},{\"id\":11,\"name\":\"int_val\",\"type\":" : "\"INT32\",\"is_key\":false,\"is_nullable\":false,\"encoding\":\"AUTO_ENCODING\"," : "\"compression\":\"DEFAULT_COMPRESSION\",\"cfile_block_size\":0,\"immutable\":false}]}," : "\"partition_schema\":{\"range_schema\":{\"columns\":[{\"id\":10}]}},\"owner\":\"$1\"," : "\"comment\":\"\",\"extra_config\":{}} not sure if this schema is reused in the other tests, but if it is, then extract it into a const e.g.: const char kTableSchema = "...."; if it's not exactly the same then try to parameterise it so it can be reused by passing it into a Substitute() http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@266 PS17, Line 266: R"({ : "name": "test2", : "schema": { : "columns": [ : {"name": "id", "type": "INT32", "is_key": true}, : {"name": "key", "type": "INT64", "is_nullable": false, "is_key": true, "comment": "range partition column"}, : {"name": "name", "type": "STRING", "is_nullable": false, "comment": "user name"} : ] : }, : "partition_schema": { : "hash_schema": { : "columns": [{"name": "key"}, {"name": "id"}], : "num_buckets": 2, : "seed": 8 : }, : "range_schema": { : "columns": [{"name": "key"}] : } : }, : "num_replicas": 1, : "owner": "root" : })", : &buf); extract to a const string if used multiple times -- To view, visit http://gerrit.cloudera.org:8080/21823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f964c4f950edfde31772cafd5c3ed5d6b87413 Gerrit-Change-Number: 21823 Gerrit-PatchSet: 17 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 03 Dec 2024 13:09:01 +0000 Gerrit-HasComments: Yes
