impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4669: [KUTIL] Add kudu util library to the build.
Date Thu, 30 Mar 2017 23:08:15 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message