Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )
Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements ...................................................................... Patch Set 11: (8 comments) Updates made per the 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: KUDU-2459: add placeholder names to some CREATE TABLE statements > We typically format commit messages as: Done 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: string orig_name = l.data().name(); : if (!orig_name.empty()) { > Don't need "std::" prefixes; there's a "using std::string" at the top of th Done http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@268 PS5, Line 268: //Ensuring non-zero-length tablename > Do !orig_name.empty() instead; it's more idiomatic. Done 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 toget Combining these if-conditions exceeds the total code line length requirement. I broke it into separate conditions for more readability purposes. I have added code comments. I can still concatenate if you feel that's better, but with the line breaks, the logic appears more complex to follow. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@270 PS5, Line 270: //Ensuring that the tablename length does not exceed 128 characters > Add a space between the operator (>=) and each of the operands (orig_name[0 Done http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@272 PS5, Line 272: //Ensuring first character of tablename is alphabetic : if (find_if(orig_name.begin(), orig_name.end(),[](char c) { > Reformat this so it's more obvious that "return ..." is the body of a lambd Done http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@275 PS5, Line 275: //For every character in the name, it checks whether the character is alphanumeric > Won't this cause HandleTablePage to return early and not finish outputting I have removed the return statement. 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? No, this is the actual kudu tablename, it needs to be carried here as is. -- 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: 11 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: Wed, 01 Aug 2018 02:41:08 +0000 Gerrit-HasComments: Yes
