Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )
Change subject: Code update for KUDU-2459 ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG@7 PS5, Line 7: Code update for KUDU-2459 We typically format commit messages as: <bug number if applicable>: <short description of fix> <longer description of fix> So in your case it could be: KUDU-2459: add placeholder names to some CREATE TABLE statements <explain the problem. explain the solutions considered, and why this one was adopted> Remember to wrap each line to 80 characters (https://kudu.apache.org/docs/contributing.html#_line_length). http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@266 PS5, Line 266: std::string impala_name = "<placeholder>"; : std::string orig_name = l.data().name(); Don't need "std::" prefixes; there's a "using std::string" at the top of the file. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@268 PS5, Line 268: if (orig_name.length() >= 1) { Do !orig_name.empty() instead; it's more idiomatic. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269 PS5, Line 269: if (orig_name.length() <= 128) { Please combine these conditions rather than nesting more and more ifs together. Please also document the constraints in a more human-readable way via code comment. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@270 PS5, Line 270: if ((orig_name[0]>='a' && orig_name[0] <= 'z') Add a space between the operator (>=) and each of the operands (orig_name[0] and 'a'). Actually, just use ascii_isalpha() from gutil/strings/ascii_ctype.h. Below too use ascii_isalnum() instead of isalnum() from <ctype.h>. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@272 PS5, Line 272: if (find_if(orig_name.begin(), orig_name.end(),[](char c) { : return !(isalnum(c) || (c == '_')); }) == orig_name.end()) Reformat this so it's more obvious that "return ..." is the body of a lambda. Something like this: if (find_if(orig_name.begin(), orig_name.end(), [](char c) { return !(isalnum(c) || (c == '_')); }) == orig_name.end()) { ... } http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@275 PS5, Line 275: return; Won't this cause HandleTablePage to return early and not finish outputting below? This is why I think it'd still be good to decompose this into a helper method, because then it's easy to test, and you could have caught this mistake. http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache File www/table.mustache: http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache@156 PS5, Line 156: name Does this need to be replaced with a placeholder too? -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 5 Gerrit-Owner: Shriya Gupta <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Tue, 31 Jul 2018 18:19:38 +0000 Gerrit-HasComments: Yes
