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 <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
