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 <he...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
