[kudu-CR] [KUDU-754] add an environment variable for kudu client debugging to stderr

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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 <wi...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: William Li <wi...@gmail.com>
Gerrit-HasComments: Yes