impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Date Tue, 28 Mar 2017 21:30:10 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
......................................................................


Patch Set 8: Code-Review+2

(2 comments)

> (4 comments)
 > 
 > I don't think there's a new upstream version of this code in Kudu
 > (which comes from https://chromium.googlesource.com/chromium/src/base/).
 > But they made some local changes that were necessary to keep the
 > kudu library dependencies compiling.
 > 

Okay. I wasn't sure given some of the large deletions (like sparsetable.h). So you're saying
that kudu deleted that code from their gutils, it wasn't deleted upstream).

 > I think having gutil as a toolchain dependency might make some
 > sense in the future, so that it's easier to replay the patches we
 > want if we ever upgrade the base code.
 > 
 > For now, I've split the two commits a bit better: this commit only
 > imports the code (and touches CMakeLists.txt). The following commit
 > makes all the code changes required to have it compile. That way
 > only that commit needs to be checked in the future if we have to do
 > this again.

Okay, let me take a quick look at the second patch again given some code has moved between
them.

http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/gscoped_ptr.h
File be/src/gutil/gscoped_ptr.h:

PS7, Line 112: kudu
> It happened in this kudu commit:
It's just surprising to impala developers to see this, if they don't know this full history.
you'd ask yourself, "what does this code have to do with kudu?" and the answer is "nothing".

I'm fine with leaving it, just wasn't sure what was going on with it.


http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/port.h
File be/src/gutil/port.h:

PS7, Line 939: kudu
> Came with the bulk-import. Since it's commented out, I didn't investigate t
okay. similar to kudu namespace, this is just another thing that will be confusing to impala
developers, but given how remote the code is, i'm not overly concerned.  mainly just wanted
to verify there wasn't a botched merge here or something.


-- 
To view, visit http://gerrit.cloudera.org:8080/5687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message