impala-reviews mailing list archives

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

Mime
View raw message