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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
