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:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4733/3/src/kudu/util/flags-test.cc
File src/kudu/util/flags-test.cc:

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, 
&writable_file));
            : 
            :   std::string flagfile_contents = 
"--test_nondefault_ff=nondefault\n"
            :                                   "--test_default_ff=default";
            : 
            :   CHECK_OK(writable_file->Append(Slice(flagfile_contents.data(),
            :                                        
flagfile_contents.size())));
            :   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);
can use ASSERT_STR_CONTAINS


http://gerrit.cloudera.org:8080/#/c/4733/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

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


http://gerrit.cloudera.org:8080/#/c/4733/3/src/kudu/util/flags.h
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 http://gerrit.cloudera.org:8080/4733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <smyatkinma...@gmail.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to