Todd Lipcon has posted comments on this change.

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup

Patch Set 3:

File src/kudu/util/

PS3, Line 47: flagfile_path;
- member variables should end in '_'
- but this is only used in one function, so maybe just set it in the function?

PS3, Line 56:   WritableFileOptions opts;
            :   gscoped_ptr<WritableFile> writable_file;
            :   CHECK_OK(Env::Default()->NewWritableFile(opts, flagfile_path, 
            :   std::string flagfile_contents = 
            :                                   "--test_default_ff=default";
            :   CHECK_OK(writable_file->Append(Slice(,
            :   CHECK_OK(writable_file->Close());
can use WriteStringToFile for this

PS3, Line 67: std::string flagfile_flag = "--flagfile=";
            :   flagfile_flag.append(flagfile_path);
typically we'd use strings::Substitute("--flagfile=$0", flagfile_path)

Line 78:   char** casted_argv = const_cast<char**>(argv);
no need for the intermediate variable here - just do the const cast on line 80

Line 96:     EXPECT_TRUE(result.find(expected) != result.npos);
File src/kudu/util/

Line 360: string GetNonDefaultFlags(std::vector<google::CommandLineFlagInfo>& 
default_flags) {
should be a const ref

PS3, Line 366:  // This only means, that the flag have been rewritten. Doesn't 
mean, that
             :       // it is done in command line or even that it's trully 
different from
             :       // default value.
some grammar/spelling mistakes:

// This only means that the flag has been rewritten. It doesn't
// mean that this has been done in the command line, or even 
// that it's truly different from the default value.

Line 370:       for (const auto& default_flag : default_flags) {
rather than the O(n^2) loop here, I think it would be better to capture the 
default_flags in a map<string, CommandLineInfo> or unordered_map
File src/kudu/util/flags.h:

Line 52:                                default_flags);
> warning: non-const reference parameter 'default_flags', make it const or us
also wrapping here isn't necessary (we allow up to 100)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to