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 Thu, 15 Jun 2017 21:36:52 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4669: [KUTIL] Add kudu_util library to the build.
......................................................................


Patch Set 13:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5715/13/be/src/common/logging.cc
File be/src/common/logging.cc:

PS13, Line 55: namespace {
             : bool logging_initialized = false;
             : }
> That's equivalent to static bool logging_initialized = false;, right ? Not 
Yes. The idea is to make sure this can't conflict with any other variables of the same name.
This could go upstream at some point.


http://gerrit.cloudera.org:8080/#/c/5715/13/be/src/kudu/util/compression/compression_codec.cc
File be/src/kudu/util/compression/compression_codec.cc:

PS13, Line 24:  LZ4_DISABLE_DEPRECATE_WARNINGS
> Why is this not needed in Kudu code base ?
Kudu uses lz4 r130, which is older than our 1.7.5 dependency. By 1.7.5, the APIs used by Kudu
have been deprecated (but they are thin wrappers around their modern equivalents).


http://gerrit.cloudera.org:8080/#/c/5715/13/be/src/kudu/util/kudu_export.h
File be/src/kudu/util/kudu_export.h:

Line 1: 
> Is the missing ASF header intentional ?
Done


http://gerrit.cloudera.org:8080/#/c/5715/13/be/src/kudu/util/minidump.cc
File be/src/kudu/util/minidump.cc:

PS13, Line 96: namespace kudu {
> We also have breakpad enabled for Impala and this file contains logic which
I believe this is dead code, as the MinidumpExceptionHandler is not instantiated anywhere.


-- 
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: 13
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-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message