Todd Lipcon has posted comments on this change. Change subject: [KUDU-754] add an environment variable for kudu client debugging to stderr ......................................................................
Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/6736/1//COMMIT_MSG Commit Message: Line 9: read environment variable "KUDU_CLIENT_VERBOSE", calls SetVerboseLogLevel to set the specific verbose level. The InitiGoogleLoggingSafeBasic() already sets the debugging to stderr. nit: can you re-wrap and format this into full sentences with appropriate capitalization, etc? https://chris.beams.io/posts/git-commit/ has a good guide http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 5049: nit: whitespace here and below Line 5050: // Test that, if a different verbose level through environment varialble is set, it reflects to the FLAGS_v nit: please wrap (here and a few lines down as well) Line 5053: char sample_verboseLevel[] = "5"; // select a specific verbose level different from 0 no need for a separate variable here. I think it's clear what you're doing so the comment can also be removed. Line 5059: ASSERT_EQ(std::atoi(sample_verboseLevel), FLAGS_v); I think it's fine to just use '5' here literal instead of std::atoi() it above, since the test is so short it doesn't really aid clarity http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client.cc File src/kudu/client/client.cc: PS1, Line 148: kLevel this isn't a constant, so it should just be 'int level' PS1, Line 149: char const* temp const char*? Line 151: { style: { goes on the same line as 'if'. Same below. Line 152: kLevel = std::atoi(temp); use safe_strto32() from numbers.h instead, so you can check validity. Maybe LOG(WARNING) if the env var is invalid? PS1, Line 153: kLevel <= 6 why <= 6? Is there some limit on the vlog level as far as glog is concerned? http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client.h File src/kudu/client/client.h: PS1, Line 125: /// Set the logging verbose level through environment variable : void KUDU_EXPORT SetClientVerboseLevelFromEnvironmentVariable(); given this is called by the client during initialization, I don't think it's necessary to export it as a public API. If you put it here just for testing, you could move it to client-internal.h (which the tests are allowed to include but don't get shipped) Line 129: static const char* kClientVerboseEnvironmentVariable = "KUDU_CLIENT_VERBOSE"; This should probably be declared extern in the header, and defined in the .cc file. -- To view, visit http://gerrit.cloudera.org:8080/6736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iab5c7c24395c25184489200283dd38da024c07bb Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: William Li <william.li.ins...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
