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-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Date Tue, 28 Mar 2017 20:59:03 GMT
Henry Robinson has posted comments on this change.

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


Patch Set 7:

(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.

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.

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

Line 88: }
> why did these move from the header?
I moved our implementation into our BitUtil class. These went back to the .cc file where they
were originally.


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
> is that intentional?
It happened in this kudu commit:

https://github.com/apache/kudu/commit/bb6da1946ea90e3b64bb7a38d9e5751cc95c557b

"gutil: move classes used by exported client into kudu namespace"

To me it's unfortunate but benign. Since the kutil libraries use these classes (much more
than we do), we'd have to change the namespacing either here or there. I think, but am prepared
to be convinced otherwise, that it's better to have as few changes to these bulk imports as
possible.


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

PS7, Line 939: kudu
> is that expected?
Came with the bulk-import. Since it's commented out, I didn't investigate too deeply (again,
it's a place where fidelity to the source probably outweighs cleanliness concerns).


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

PS7, Line 83: 1e6
> we had intentionally changed these so that we'd be using integer operations
Done.


-- 
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: 7
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