Henry Robinson 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. I agree, it's a lot of new (unused) gflags. I filed https://issues.apache.org/jira/browse/IMPALA-5174 to see if > 2) I'm curious to know how this (and more particularly the actual code import > change) affects the binary size. Before: 391865544 After: 391795120 So somehow it's smaller (?). I'll take a closer look, but nothing to worry about right now. > 3) It'd be good if we could push some of these changes upstream if possible. Will do. The issue is that every upstream update brings in all the changes since the last rebase, which leads to more compilation or even functionality issues to resolve. So rather than try and make that process converge, I figured I'd draw a line under a known good version of kutil, and add the few changes needed to Impala. I should upstream these changes soon, though, so that next upgrade we bring them all in. 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 th Turns out this isn't necessary any more. 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, an I agree - I have tried upstreaming it but the reviewers (understandably) wanted a slightly more general solution that I didn't have time to do. I would expect this to show up in future upgrades of the kutil library. Line 331: target_link_libraries(protoc-gen-insertions gutil glog gflags protobuf protoc ${KUDU_BASE_LIBS}) > can this be added upstream? Will look into it. 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? Yes, I think so. 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 Not AFAICT. We don't call kudu's log initialization code. -- 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
