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