William Li has posted comments on this change. Change subject: [KUDU-754] add an environment variable for kudu client debugging to stderr ......................................................................
Patch Set 2: (12 comments) new patch has been submitted. http://gerrit.cloudera.org:8080/#/c/6736/1//COMMIT_MSG Commit Message: Line 9: Read environment variable "KUDU_CLIENT_VERBOSE" to get verbose level. > nit: can you re-wrap and format this into full sentences with appropriate c Done 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 Done Line 5050: // Test that, log verbose level can be set through environment varialble > nit: please wrap (here and a few lines down as well) Done Line 5053: FLAGS_v = 0; > no need for a separate variable here. I think it's clear what you're doing Done Line 5059: > I think it's fine to just use '5' here literal instead of std::atoi() it ab Done http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client.cc File src/kudu/client/client.cc: PS1, Line 148: etClie > this isn't a constant, so it should just be 'int level' Done PS1, Line 149: int32_t level = 0 > const char*? Done Line 151: if (env_verbose_level != NULL) { > style: { goes on the same line as 'if'. Same below. Done Line 152: if (safe_strto32(env_verbose_level, &level) && (level >=0)) { > use safe_strto32() from numbers.h instead, so you can check validity. Maybe Done PS1, Line 153: el(level); > why <= 6? Is there some limit on the vlog level as far as glog is concerned I was reading the comments from client.h at line 110 paragraph: ".. the highest verbosity level used in Kudu is 6, ....". I leave it open for the upper bound for now. If later you want to pick a big enough number to just set a limitation, we can do that too. http://gerrit.cloudera.org:8080/#/c/6736/1/src/kudu/client/client.h File src/kudu/client/client.h: PS1, Line 125: /// Set signal number to use internally. : /// > given this is called by the client during initialization, I don't think it' Done Line 129: /// this advanced API can help workaround conflicts. > This should probably be declared extern in the header, and defined in the . Done -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: William Li <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: William Li <[email protected]> Gerrit-HasComments: Yes
