Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4669: [KUTIL] Add kudu_util library to the build. ......................................................................
Patch Set 5: (5 comments) 1) Brining in all the gflags is maybe concerning. will any conflict with ours? even if not, they will crowd our already large number of gflags. perhaps we need to look more closely at them. 2) I'm curious to know how this (and more particularly the actual code import change) affects the binary size. 3) It'd be good if we could push some of these changes upstream if possible. Some additional comments inline. http://gerrit.cloudera.org:8080/#/c/5715/5/be/CMakeLists.txt File be/CMakeLists.txt: Line 315: maintenance_manager_proto It might be nice to keep kudu specific libs in a separate list and merge that list in. http://gerrit.cloudera.org:8080/#/c/5715/5/be/src/kudu/util/CMakeLists.txt File be/src/kudu/util/CMakeLists.txt: Line 208: if(NOT NO_NVM_SUPPORT) I think the NO_NVM_SUPPORT flag would probably be useful to add to Kudu, and of course if its upstream it's less to maintain in our codebase in the future. Line 331: target_link_libraries(protoc-gen-insertions gutil glog gflags protobuf protoc ${KUDU_BASE_LIBS}) can this be added upstream? http://gerrit.cloudera.org:8080/#/c/5715/5/be/src/kudu/util/env.cc File be/src/kudu/util/env.cc: Line 10: #include "kudu/util/status.h" seems like they're missing this include, can this be added upstream? http://gerrit.cloudera.org:8080/#/c/5715/5/be/src/kudu/util/logging.cc File be/src/kudu/util/logging.cc: Line 47: DECLARE_string(log_filename); what does this mean for our log files? will this code start writing to our log? -- To view, visit http://gerrit.cloudera.org:8080/5715 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8da798ee55506d6f969416b17c191eb03cb215f5 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
