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 13: (7 comments) > (7 comments) > > I filed KUDU-2523 for the test failure, which looks unrelated to > your patch. Yep, thank you, I wasn't sure why a commit message update on a successful build would warrant/fail a debug build test. I have made the other updates too. http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12 PS12, Line 12: ablename > Across this commit message (and the code), you're writing both "table name" Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17 PS12, Line 17: deriv > derive Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19 PS12, Line 19: W > Nit: space before "We" Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21 PS12, Line 21: invalid tablenames have been identified and updated by a placeholder : tablename for end user to replace with a valid tablename. The rules to > Nit: you have trailing whitespace on these two lines. 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@269 PS5, Line 269: // 2. Length must not exceed 128 characters. > I don't think a combined if statement with line breaks is so bad: Done http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268 PS12, Line 268: // 1. Must not be empty. > Style nit: format single-line comments as "// foo bar" not "//foo bar". Done http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270 PS12, Line 270: // 3. First character must be alphabetic. > I think these comments would be more readable if grouped into a single bloc Done -- 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: 13 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 18:16:20 +0000 Gerrit-HasComments: Yes
