Alexey Serbin has posted comments on this change. Change subject: [KUDU-754] add an environment variable for kudu client debugging to stderr ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/6736/2/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: PS2, Line 259: e nit: please add period (.) in the end of the sentence. PS2, Line 260: Client nit: probably, it's worth dropping the 'Client' part since it's already in the 'client' namespace. Also, what do you think about using more or less ubiquitous abbreviations (like in other parts of the source code: search for EnvVar) and name it just SetVerboseLevelFromEnvVar() ? The same for the kClientVerboseEnvironmentVariable. Line 262: // Verbose log Environment Variable Name ditto PS2, Line 263: Client nit: if it's already in the 'client' namespace, may be it's possible to drop that 'Client' part? http://gerrit.cloudera.org:8080/#/c/6736/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 5057: ASSERT_EQ(5, FLAGS_v); Does it make sense to add a case for negative and non-parsable values to verify that current FLAGS_v value is not touched? http://gerrit.cloudera.org:8080/#/c/6736/2/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 21: #include <memory> nit: please add #include <cstdlib> since std::getenv() is from that header. PS2, Line 151: NULL nit: nullptr PS2, Line 155: environment variable nit: does it make sense to output the name of the variable? -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: William Li <[email protected]> Gerrit-HasComments: Yes
