You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/01/05 21:15:26 UTC

[kudu-CR] Add Google Breakpad support to Kudu

Hello Dinesh Bhat, Lars Volker, Adar Dembo,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5620

to review the following change.

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

When a process crashes, Breakpad writes a small dump file called a
minidump to disk. Since these files are typically only a few MB in size,
this allows users to share critical crash information with developers
for offline analysis.

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process.

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do releases).
* Add "-system $PREFIX/includes/breakpad" to the build command line,
  because breakpad does not properly namespace its includes (however it
  installs them to their own directory).
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
  The modifications mostly consist of removing use of boost::filesystem
  and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
  startup routines.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M .ycm_extra_conf.py
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
16 files changed, 501 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 37: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 37
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 164: Attempting to delete it
> Nit: would be nice to print this in next line separately.
I disagree -- it's easier for grep purposes to keep individual events described in one line of logging.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.

Dinesh Bhat has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(4 comments)

I forgot to press send on this friday. 
Thinking out loud, I am trying to see what's the real advantage breakpad can give us over gdb corefile ? If we could squeeze in more info about kudu components - things like which tablet_id caused the crash - in the crash info, it might be one way of gaining control over how we want to present crash dump info in a useful manner for debugger.

http://gerrit.cloudera.org:8080/#/c/5620/3//COMMIT_MSG
Commit Message:

Line 36: 
It would be good to include a section on how-to go about debugging the minidump file. For eg, which tool to use, should we rely on another tool to convert minidump to corefile and run gdb etc.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 160: Slice
Do we explicitly want to include #include "kudu/util/slice.h" for this ?


PS3, Line 164: Attempting to delete it
Nit: would be nice to print this in next line separately.


PS3, Line 221: Impala
s/Impala/kudu ?


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has submitted this change and it was merged.

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

Breakpad creates minidumps upon crash, which are small files that
include stack-based information for each thread. These do not include
heap information, so they complement, not replace, core files for
debugging purposes.

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

When a process crashes, Breakpad writes a small dump file called a
minidump to disk. Since these files are typically only a few MB in size,
this allows users to share critical crash information with developers
for offline analysis.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

1. The minidump files are relatively small, potentially comparable in
size to log files, and the log directory is a place an administrator
would look in when debugging.
2. This convention is consistent with what Apache Impala (incubating)
does for its minidump files.

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux vs Apple.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Reviewed-on: http://gerrit.cloudera.org:8080/5620
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/client/symbols.map
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/logging.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
30 files changed, 1,028 insertions(+), 85 deletions(-)

Approvals:
David Ribeiro Alves: Looks good to me, approved
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 38
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 24:

> I don't see any reason why the client would link these symbols.

Maybe it's because of the static variable current_num_instances_. But, again, doesn't really matter.

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#3).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

When a process crashes, Breakpad writes a small dump file called a
minidump to disk. Since these files are typically only a few MB in size,
this allows users to share critical crash information with developers
for offline analysis.

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process.

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
  releases).
* Add a "hack" to the breakpad thirdparty installation routine to
  forcibly add a breakpad/ prefix to the breakpad header files. This
  frees us from having to modify our include path when building.
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
  The modifications mostly consist of removing use of boost::filesystem
  and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
  startup routines.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
16 files changed, 521 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 30: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 30
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#13).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
25 files changed, 935 insertions(+), 29 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/13
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#25).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/client/symbols.map
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/logging.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
30 files changed, 1,012 insertions(+), 84 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/25
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 25
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.

Lars Volker has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 85: It logs the event before breakpad crashes
            : // the process
> I hadn't considered this, but it turned out to be an issue.
Yes, we had to do the same (https://github.com/cloudera/native-toolchain/commit/df2c2fa07c5a2394f8c6581c770201e19312f397). I didn't remember until I read this comment though. :(


Line 118:   minidump_exception_handler->WriteMinidump(FLAGS_minidump_path, DumpCallback, nullptr);
> Hrmm. I was interpreting it the other way, but I think you're right... this
Good catch! Looking at the implementation, WriteMinidump() calls memset(), which is not async-signal-safe. The Minidump itself will be written from a fork()'ed child, so I assume the window for the race is rather small. However, I'm not sure I completely understood the severity of this, so I will reach out to you in an email. I created IMPALA-4796 to track this.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 32: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#10).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
24 files changed, 929 insertions(+), 26 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/10
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#9).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
The modifications mostly consist of removing use of boost::filesystem
and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/9
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#19).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
27 files changed, 939 insertions(+), 36 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/19
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 31: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 31
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 37: Code-Review+2

haven't actually tested this last rev on mac, assuming there weren't any new breakages

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 37
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 6:

(21 comments)

http://gerrit.cloudera.org:8080/#/c/5620/6//COMMIT_MSG
Commit Message:

PS6, Line 29: default
> Nit: probably don't need 'default' here (since the minidump files are writt
Done


PS6, Line 51: * By default, invoke previously-installed signal handler if installed.
            : * Install SIGUSR1 handler to create minidump on demand.
> These two aren't in any more, right?
The first one never left; the second one is back in


http://gerrit.cloudera.org:8080/#/c/5620/6/CMakeLists.txt
File CMakeLists.txt:

Line 915: if (NOT APPLE)
> Oh, I thought it did work on macOS. Bummer.
It's possible, but I didn't do the breakpad build work to make it happen. We'd have to figure out how to make the breakpad client library build on Mac.

They only provide Xcode files to build it; the Makefiles specifically exclude Mac and making breakpad work on Mac seems very low priority-wise.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

Line 52: #if !defined(__linux__)
> Wouldn't it be cleaner to just not build the test for macOS? That is, patch
I think this is more discoverable if someone wants to come around later and add MacOS breakpad support. But yeah, the other way is a little neater. I don't have a strong preference so I'll take your suggestion.


Line 68:   master->process()->Kill(SIGSEGV);
> Maybe run the tserver/master test in a loop and try each signal for which y
I do something like that in a different test.


Line 72: TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) {
> I thought this functionality was removed?
I don't think it needs to be removed now. But I posted this patch revision before I tried to fix it.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

PS6, Line 318:   if (FLAGS_logtostderr) {
             :     return Status::OK();
             :   }
> This is no longer correct.
good catch


PS6, Line 326:   // Same with minidumps.
> Nit: could you work this into the previous comment instead?
Done


PS6, Line 329: excess-glog-deleter"
> Should generalize the thread name.
Done


PS6, Line 330: excess_glog_deleter_thread_
> Probably rename this too.
Done


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump-test.cc
File src/kudu/util/minidump-test.cc:

PS6, Line 60: #if !defined(__linux__)
            :   return;
            : #endif
> Again, I think we should just not build the test on macOS. Also, shouldn't 
Disabled building this file on MacOS.


Line 67:     CHECK_EQ(1, 0);
> Maybe just invoke the FAIL() macro?
Not sure that would be fatal but I switched this to abort().


Line 78:     kill(getpid(), SIGUSR1);
> I thought you removed the SIGUSR1 functionality?
See my other comments


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS6, Line 28: #if defined(__linux__)
            : #include <breakpad/client/linux/handler/exception_handler.h>
            : #include <breakpad/common/linux/linux_libc_support.h>
            : #endif // defined(__linux__)
> How about just not compiling minidump.cc for macOS? Or providing a second m
It sounds good in theory but then you have to worry about gflag DEFINEs (do they just not get defined on Mac or do we have to put them in the stub file too?) and we end up with extra build stuff. Having #ifdefs in here doesn't seem that bad to me.


Line 66:     LOG(ERROR) << "Minidumps are not supported on this platform";
> Is it actually safe to LOG(ERROR) in this context? Has glog been initialize
It should just go to stderr. But I'll simply remove this message anyway.


Line 98: DEFINE_bool(minidump_invoke_previous_signal_handler, true,
> What's the motivation for this? Why ever set it to false?
I was originally unsure whether this would work. I don't think we need to keep the flag, so I'll remove it.


PS6, Line 264:   // Under Linux we always set up a signal handler for SIGUSR1. This is so that
             :   // when minidumps are not enabled, we will not crash, we will simply no-op.
> I don't understand the motivation (though again, maybe it's a moot point); 
For consistency: To avoid a signal being either safe to send with one command line flag set or fatal with another command line flag set. It's better to make it so that it's always safe to send the signal to the process. The way it's written, in one case it's a no-op and in the case it generates a minidump.


Line 337:   if (max_minidumps <= 0) return Status::OK();
> Nit: format this simple if/return the way the FLAGS_enable_minidumps one is
Done


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

PS6, Line 35: r if it
            : // received a USR1 signal
> I thought you removed this?
See elsewhere


Line 41: // Only one instance of this class may be instantiated at a time.
> Having reviewed the singleton-based approach, it seems more complicated tha
I attempted using a GoogleOnce-based approach before rewriting this as a singleton. It was painful for the following reasons:

1. I wanted to put the minidump files in a location that would be cleaned up after each test. In order to do that in a test case I would have to manage the creation / cleanup in the test case itself.
2. Without non-gflag persistent state to rely on it became difficult to know / remember where we were writing our minidump files to across test invocations. It was much cleaner to have some object state independent of gflags.

Using a singleton results in more intuitive and predictable test code than what we were doing before, which was modifying gflags to keep track of the location of the minidump directory and then reading it back later.

I went a step further and used a singleton with a lifecycle so that I wouldn't have to manage cleaning up the minidump directory between tests and bringing up the singleton at the test case level instead of the test level. It didn't seem to add much additional complexity to achieve that.

It's possible to roll back some of the lifecycle and make it a "live forever" singleton but I think it's pretty clean as written now.


PS6, Line 56: Should not be called from a crash context
            :   // because it uses the heap.
> Should not be called from a signal handler either, right?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 6:

(22 comments)

> The right way to do it would probably be to have a signal-handling thread.

Another option is to write a minidump in response to a krpc request. That means it could be requested remotely, which is both good and bad.

http://gerrit.cloudera.org:8080/#/c/5620/6//COMMIT_MSG
Commit Message:

PS6, Line 29: default
Nit: probably don't need 'default' here (since the minidump files are written to whatever the log directory is, regardless of whether it's the default value or not).


PS6, Line 51: * By default, invoke previously-installed signal handler if installed.
            : * Install SIGUSR1 handler to create minidump on demand.
These two aren't in any more, right?


http://gerrit.cloudera.org:8080/#/c/5620/6/CMakeLists.txt
File CMakeLists.txt:

Line 915: if (NOT APPLE)
Oh, I thought it did work on macOS. Bummer.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

Line 52: #if !defined(__linux__)
Wouldn't it be cleaner to just not build the test for macOS? That is, patch CMakeLists.txt instead of #ifdef here?


Line 68:   master->process()->Kill(SIGSEGV);
Maybe run the tserver/master test in a loop and try each signal for which you expect breakpad to generate a dump?


Line 72: TEST_F(MinidumpGenerationITest, TestCreateMinidumpOnSIGUSR1) {
I thought this functionality was removed?


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

PS6, Line 318:   if (FLAGS_logtostderr) {
             :     return Status::OK();
             :   }
This is no longer correct.


PS6, Line 326:   // Same with minidumps.
Nit: could you work this into the previous comment instead?


PS6, Line 329: excess-glog-deleter"
Should generalize the thread name.


PS6, Line 330: excess_glog_deleter_thread_
Probably rename this too.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump-test.cc
File src/kudu/util/minidump-test.cc:

PS6, Line 60: #if !defined(__linux__)
            :   return;
            : #endif
Again, I think we should just not build the test on macOS. Also, shouldn't the second test be conditioned?


Line 67:     CHECK_EQ(1, 0);
Maybe just invoke the FAIL() macro?


Line 78:     kill(getpid(), SIGUSR1);
I thought you removed the SIGUSR1 functionality?


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 118: // Write two null-terminated strings and a newline to both stdout and stderr.
> Good catch! Looking at the implementation, WriteMinidump() calls memset(), 
I left a bunch of comments to the effect of "but I thought we were removing the SIGUSR1 functionality?" after forgetting that you intended to remove it but haven't yet. You can ignore them.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS6, Line 28: #if defined(__linux__)
            : #include <breakpad/client/linux/handler/exception_handler.h>
            : #include <breakpad/common/linux/linux_libc_support.h>
            : #endif // defined(__linux__)
How about just not compiling minidump.cc for macOS? Or providing a second minidump_stubs.cc file that defines the public methods to no-op?


Line 66:     LOG(ERROR) << "Minidumps are not supported on this platform";
Is it actually safe to LOG(ERROR) in this context? Has glog been initialized by this point?


Line 98: DEFINE_bool(minidump_invoke_previous_signal_handler, true,
What's the motivation for this? Why ever set it to false?


PS6, Line 264:   // Under Linux we always set up a signal handler for SIGUSR1. This is so that
             :   // when minidumps are not enabled, we will not crash, we will simply no-op.
I don't understand the motivation (though again, maybe it's a moot point); if minidumping is disabled, why shouldn't SIGUSR1 kill the process as it normally does?


Line 337:   if (max_minidumps <= 0) return Status::OK();
Nit: format this simple if/return the way the FLAGS_enable_minidumps one is formatted? Or the other way around?


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

PS6, Line 35: r if it
            : // received a USR1 signal
I thought you removed this?


Line 41: // Only one instance of this class may be instantiated at a time.
Having reviewed the singleton-based approach, it seems more complicated than the previous one. How exactly does it simplify testing? Seems like encapsulation could have been achieved previously with a test fixture whose constructor initialized minidump writing and whose destructor uninitialized it.

If you choose to continue down the singleton path, can some of this logic be simplified with gutil/once.h or util/once.h?


PS6, Line 56: Should not be called from a crash context
            :   // because it uses the heap.
Should not be called from a signal handler either, right?


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#32).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/32
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 34:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/34/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 86:   __attribute__((unused)) std::atomic<bool> user_signal_handler_thread_running_;
Hmm, I thought this typically goes after the variable declaration, not before.

Also, use ATTRIBUTE_UNUSED from gutil/port.h, I think that's preferable.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#37).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/37
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 37
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................

Patch Set 27:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:

Line 26: # breakpad
> Can we wildcard some of these? Maybe ConvertUTF* and my_*?
Done

http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 217: google::InstallFailureFunction(&AbortFailureFunction);
> Do you even need the redirection through AbortFailureFunction? abort() has
Yeah, just passing abort works, but I thought it was helpful to have it jump through AbortFailureFunction so it would be obvious that it was triggered through this code path.

http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

Line 432: ResetSigPipeHandlerToDefault();
> Why do we only reset SIGPIPE to SIG_DFL and not every signal? What about SI
I added a short comment, but it's for the following reasons:
1. The shell or parent process invoking Kudu may set the signal disposition to ignore and they might know better than us what they want, e.g. for HUP.
2. As far as I could find, we only explicitly set the signal disposition to IGN for PIPE; we don't ignore any other signals.
3. Setting the signal handler to DFL is only necessary if it was previously set to IGN; if it was set to be caught then execve() automatically sets it to DFL.

--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 27
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 33:

just posted a couple of fixes that make this compile in macOS. mike feel free to grab and fix whatever you thing needs fixing.

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 33
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#2).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

When a process crashes, Breakpad writes a small dump file called a
minidump to disk. Since these files are typically only a few MB in size,
this allows users to share critical crash information with developers
for offline analysis.

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process.

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do releases).
* Add "-system $PREFIX/includes/breakpad" to the build command line,
  because breakpad does not properly namespace its includes (however it
  installs them to their own directory).
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
  The modifications mostly consist of removing use of boost::filesystem
  and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
  startup routines.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M .ycm_extra_conf.py
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
16 files changed, 494 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5620/1//COMMIT_MSG
Commit Message:

Line 28:   because breakpad does not properly namespace its includes (however it
> I'm pro-hack here because I think the possibility for include conflicts is 
Done


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:

Line 208:   breakpad_client
> Not sure. I will check.
AFAIK it doesn't make it into the client library. I got empty output from running this command in release mode:

  nm -a lib/exported/libkudu_client.so | egrep "breakpad|Minidump"


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS1, Line 220: CreateDirsRecursively
> right, but for other paths (log dir, data dirs, etc) we don't recursively c
Although I don't think it's a huge risk I can see the argument. Removed.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5620/1//COMMIT_MSG
Commit Message:

Line 28:   because breakpad does not properly namespace its includes (however it
> I'm not sure what you mean by "namespace" here. You don't mean namespace as
Yes, directory prefix. I actually wrote a script to do preprocessing of the breakpad source to include a "breakpad/" prefix in their #includes and then dropped it because I thought it was kind of dirty. Ideally, we could convince them to add a prefix but it seems like a tall order since people would also have to change Firefox and Chromium code after doing that. But if you think that hack is better then I can bring it back.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

Line 56:   ASSERT_TRUE(found) << "Unable to find minidump after " << timeout.ToString();
> could you restructure this test using AssertEventually(...) to be a bit eas
Nice, done.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:

Line 208:   breakpad_client
> does putting this stuff in util/ mean that we're ending up linking unnecess
Not sure. I will check.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 99:     const int path_len = my_strlen(path);
> I don't think it's calling into libc that's problematic, is it? I think the
This is what it says in https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/linux/linux_libc_support.h ...

  // This header provides replacements for libc functions that we need. We if
  // call the libc functions directly we risk crashing in the dynamic linker as
  // it tries to resolve uncached PLT entries.


PS1, Line 220: CreateDirsRecursively
> we sure that recursive create is what we want vs just creating the parent a
Personally I think it makes sense because they can specify an arbitrary path for FLAGS_minidump_path.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 26: Status RegisterMinidump(const char* cmd_line_path);
> seems odd to see a C string here instead of std::string
Ah, missed this from the original Impala source. Done.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 27:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:

Line 26:     # breakpad
Can we wildcard some of these? Maybe ConvertUTF* and my_*?


http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 217:   google::InstallFailureFunction(&AbortFailureFunction);
Do you even need the redirection through AbortFailureFunction? abort() has the same signature, so maybe you can pass it directly here?


http://gerrit.cloudera.org:8080/#/c/5620/27/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

Line 432:     ResetSigPipeHandlerToDefault();
Why do we only reset SIGPIPE to SIG_DFL and not every signal? What about SIGUSR1?


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 27
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#36).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/36
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 36
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#14).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/14
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#17).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
26 files changed, 939 insertions(+), 30 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/17
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#30).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/30
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 30
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5620/1//COMMIT_MSG
Commit Message:

Line 28:   because breakpad does not properly namespace its includes (however it
> Yes, directory prefix. I actually wrote a script to do preprocessing of the
I'm pro-hack here because I think the possibility for include conflicts is kind of high. I also like the simplicity that -I$PREFIX/include/ is sufficient to get the thirdparty stuff properly.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS1, Line 220: CreateDirsRecursively
> Personally I think it makes sense because they can specify an arbitrary pat
right, but for other paths (log dir, data dirs, etc) we don't recursively create them. I think typically our policy should be "we can create our configured directories and things below them, but nothing above them". i.e if you configure log_dir=/tmp/log that's OK and we'll create it, but if you configure log_dir=/data/3/kudu-logs/ and it turns out data/3 doesn't exist, we shouldn't go and create it (because it's probably a typo and we'd end up on an incorrect mount point, etc).

I seem to recall way back when (and maybe still today) that HDFS would always recursively create the configured data directories. It also didn't strip leading whitespace from the configuration value. So I accidentally configured a data dir to " /home/todd/foo/bar" and that turned into a relative path "/my/working/directory/\ /home/todd/foo/bar" and it took me an hour to figure out where my data was getting written. Maybe the example is a little contrived, but hopefully you can see the point.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#7).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process. This was
chosen for the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
The modifications mostly consist of removing use of boost::filesystem
and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
22 files changed, 937 insertions(+), 9 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/7
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#4).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process. This was
chosen for the following reasons:

Changes in this patch:

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
21 files changed, 687 insertions(+), 9 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/4
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 37:

I tested this latest rev on macOS El Capitan and Ubuntu xenial and both builds pass.

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 37
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Mike Percy, Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#33).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/33
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 33
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.

Lars Volker has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

Line 66: 
You might want to add a test for writing minidumps on SIGUSR1, especially since that would kill the process if the signal handler hasn't been set up correctly.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 56: nly be written when a daemon exits "
            :     "unexpectedly,
This is actually not true anymore (but used to be), even for Impala, since sending SIGUSR1 will trigger a Minidump (I opened IMPALA-4736).


Line 207:   if (!FLAGS_enable_minidumps || FLAGS_minidump_path.empty()) return Status::OK();
If you skip registering the signal handler for SIGUSR1 here and someone sends that signal, then the process will die. I noticed this while reviewing your code and opened IMPALA-4737 to track it on our end.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 118:   // A comment from linux_libc_support.h is reproduced here:
> This is the USR1 signal handler. I'll rename it and add some comments to cl
My initial read was that "compromised context" meant "in a signal handler, where heap state may be inconsistent". You're saying it's better than that; that we're fine as long as the process hasn't corrupted itself? So, translated to signals, it's safe in SIGUSR1 but not in SIGABRT, SIGILL, SIGBUS, SIGFPE, or SIGSEGV?


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#21).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
27 files changed, 953 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/21
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#24).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/logging.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
28 files changed, 927 insertions(+), 42 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/24
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(10 comments)

Does it work on OS X?  Consider adding corresponding ifdefs at least at the first pass to allow this to compile on OS X.

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

PS3, Line 21: #include <glog/logging.h>
nit: is it needed?


Line 21: #include <glog/logging.h>
nit: consider adding <gtest/gtest.h>


PS3, Line 22: #include <glog/stl_logging.h>
nit: is it needed?


PS3, Line 24: #include "kudu/gutil/strings/util.h"
nit: is it needed?


PS3, Line 26: #include "kudu/util/logging.h"
nit: is it needed?


Line 28: #include "kudu/util/subprocess.h"
nit: consider including "kudu/util/monotime.h" (for MonoDelta)


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:

PS3, Line 60: argv[0]
nit: consider adding a var for argv[0] and using it around instead.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 57: Set to empty to "
            :     "disable writing minidump files.
I see there is also 'enable_minidumps' flag which is the explicit control flag for this behavior.  From the usability point, I think it's better to have only one flag controlling minidumps generation, if possible.  Would be that a problem generating/writing minidumps when this property were effectively empty?


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

PS3, Line 284: F_ALL
What is F_ALL?  I.e., where is it defined and why does it affect only the breakpad component?


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

Line 244: 
nit: an extra-line, seems like a random change


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 24: Code-Review+2

Looks good.

The test failure in client_symbol-test.sh is likely because we need to hide the breakpad symbols; see src/kudu/client/symbols.map. I don't know under what circumstances symbols in util/ wind up in the client vs. not; in this case, I don't see any reason why the client would link these symbols. But if we hide them, it doesn't really matter. The only effect is that the client library gets a little larger.

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(32 comments)

http://gerrit.cloudera.org:8080/#/c/5620/3//COMMIT_MSG
Commit Message:

Line 7: Add Google Breakpad support to Kudu
What does breakpad support mean for regular core dumps? Are they no longer enabled? Should we disable them? Are they even still useful? I'd like to see a paragraph here describing how breakpad and regular core dumps co-exist (or not).


PS3, Line 12: part
Nit: is part


PS3, Line 17: critical crash information
Since your audience here is Kudu developers, it would be nice to add a touch more detail on what information we're talking about. With a core dump I can get a backtrace of every thread, all thread stacks, and the heap. What can I get with a minidump?


Line 21: the default log directory for a given Kudu daemon process.
Why was the log directory chosen as a suitable default home?


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 1246:       return IOError(Substitute("Unable to canonicalize $0", path), errno);
Maybe this was intended for the path_util patch? Doesn't really matter though.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 48: DEFINE_bool(enable_minidumps, true, "Whether to enable minidump collection upon process crash");
Should also tag it with evolving, I think.


Line 50: TAG_FLAG(enable_minidumps, runtime);
How is this safe for changing at runtime? If the initial value was false, RegisterMinidump() will return early, and a subsequent change to true will have no effect.


PS3, Line 57: Set to empty to "
            :     "disable writing minidump files.
> I see there is also 'enable_minidumps' flag which is the explicit control f
Agreed with Alexey; having two ways to disable minidumps (enable_minidumps=false and minidump_path="") is confusing.

If an empty string is invalid input, enforce that with a gflag validator (see RegisterFlagValidator).


PS3, Line 62: Set to 0 to keep all minidump files
Shouldn't we allow 0 for "delete all outstanding minidumps at startup", and then reserve -1 for "keep all"?


Line 76: static google_breakpad::ExceptionHandler* minidump_exception_handler = nullptr;
Our convention is to prefix global variables with g_.


PS3, Line 84: by breakpad 
Nit: drop this; it's implied by "Callback for breakpad".


PS3, Line 85: It logs the event before breakpad crashes
            : // the process
Oh, so breakpad crashes the process now? Previously glog was doing that for us; does this mean the CHECK event (failure log + backtrace) in the glog no longer exists?


PS3, Line 97: sizeof(msg) / sizeof(msg[0]) - 1;
If we're ok calling into my_strlen() for the path, let's use it here too for consistency (and because this expression is...weird).


PS3, Line 99: libc
Would be good to say _why_ we shouldn't call into libc here.


Line 118:   minidump_exception_handler->WriteMinidump(FLAGS_minidump_path, DumpCallback, nullptr);
You sure this is safe to call from a signal handler? The docs (https://chromium.googlesource.com/external/google-breakpad/src/+/master/client/linux/handler/exception_handler.h) say:

  // Note that this method is not supposed to be called from a compromised
  // context as it uses the heap.


Line 137:   Env* env = Env::Default();
You can get this from the caller.


PS3, Line 139:   // Search for minidumps. There could be multiple minidumps for a single second.
             :   multimap<int, string> timestamp_to_path;
FWIW, I think the approach used in the glog cleanup thread is nicer: a vector<pair<time_t, string>> that is sorted using std::sort. Then you don't have to worry about collisions dropping items from the map, and thus mtime's time granularity (1s on HFS+, far less on Linux filesystems) is probably good enough?


Line 147:     unique_ptr<SequentialFile> in;
I think this is overkill. We don't verify that files that with glog-looking filenames are actually glog files before deleting them; why do we need to do for minidumps? Besides, the "failure policy" here deletes the files instead of ignoring them. What's the intent here?

On a second pass, I see that we're reading the header in order to get the timestamp out of it. Can't we just use mtime on the file itself instead? Minidumps are read-only once created, right; shouldn't that be safe enough?


Line 158:     constexpr int kHeaderSize = sizeof(MDRawHeader);
So breakpad doesn't namespace its classes either? Would have expected this to be google_breakpad::MDRawHeader.


Line 203:   static bool registered = false;
Maybe you can use a google once on minidump_exception_handler instead?


PS3, Line 215:   if (!env->FileExists(FLAGS_minidump_path)) {
             :     RETURN_NOT_OK(env->CreateDir(FLAGS_minidump_path));
             :   }
Maybe CreateDirRecursively() here, so that it probably handles symlinks and errors out if the path exists as a regular file? Unless you explicitly want an error when --minidump_path=/a/b/c/d and only /a/b exists...


PS3, Line 221: Impala
> s/Impala/kudu ?
I think the intent was to explain that this behavior (in kudu/util/minidump.cc) is consistent with Impala's behavior (in Impala's repo).


PS3, Line 225:   // Create the directory if it is not there. The minidump doesn't get written if there is
             :   // no directory.
             :   if (!env->FileExists(FLAGS_minidump_path)) {
             :     RETURN_NOT_OK(env->CreateDir(FLAGS_minidump_path));
             :   }
Here too? Maybe combine them into one CreateDirRecursively() call, which is more clear anyway.


PS3, Line 242: because usually only process crashes will trigger the
             :   // creation of new minidump files (they are also created on SIGUSR1).
As you say, SIGUSR1 breaks that guarantee. What about doing what we did for logging and having a background thread that peridiocally checks the number of minidumps and removes old ones? We could even reuse the same thread (it sort of makes sense; a minidump is a form of logging). See StartExcessGlogDeleterThread() in server_base.h.


PS3, Line 252: << "."
Why the period at the end? Seems unusual for a Kudu LOG statement.


PS3, Line 259: desc, FilterCallback, DumpCallback, nullptr, true, -1);
Would be nice to reformat this so that one arg is on each line, then you can add inline comments explaining what each arg means (especially for nullptr, true, and -1).


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 28: Status RegisterMinidump(const std::string& cmd_line_path);
If this is just a way to get the executable's argv[0], you can use gflags' ProgramInvocationShortName() inside the function itself. AFAICT breakpad registration happens after gflags so it should be safe.

Also, it would be nice to see some non-integration tests for this. For example, would it be possible to call RegisterMinidump in a death-test, crash, and verify that a dump was written?


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

Line 563:   make -j$PARALLEL install
Add $EXTRA_MAKEFLAGS too; see above.


Line 573:     xargs perl -p -i -e '@pre = qw(client common google_breakpad processor third_party); for $p (@pre) { s{#include "$p/}{#include "breakpad/$p/}; }'
Could you add perl to REQUIRED_TOOLS in preflight.py? This is the second usage (not the first), but we should do it anyway.


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

PS3, Line 284: F_ALL
> What is F_ALL?  I.e., where is it defined and why does it affect only the b
Agreed; did you mean F_UNINSTRUMENTED here and F_TSAN below?


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/scripts/make-breakpad-src-archive.sh
File thirdparty/scripts/make-breakpad-src-archive.sh:

Line 37: VERSION=${1:-$(git rev-parse HEAD)}
Nit: call this REVISION to be consistent with LSS_REVISION (and to make it clear that it's a git hash, not a tag or something).


Line 41: git archive --format=tar --prefix=$NAME/ $VERSION | (cd .. && tar xf -)
Is this a fancy way to copy and rename the tree to another location?


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#18).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
26 files changed, 937 insertions(+), 30 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/18
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 85: It logs the event before breakpad crashes
            : // the process
> Yes, we had to do the same (https://github.com/cloudera/native-toolchain/co
Lars, thanks this makes me feel better about having to backport that patch. :)


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................

Patch Set 21:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/5620/20/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:

PS20, Line 500: ls -al $TEST_TMPDIR
: find $TEST_TMPDIR
Maybe just "find $TEST_TMPDIR -ls" would yield the same thing as both of these?

http://gerrit.cloudera.org:8080/#/c/5620/21/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 51: import static org.junit.Assert.assertTrue;
Unused?

http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:

Line 62: CHECK_OK(BlockSigUSR1()); // Must be called before logging threads started.
Can we do this within InitGoogleLoggingSafe()? It'd affect a few more processes that way (mostly experiments and tpch stuff), but I think that's OK, and it'd be less fragile.

http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump-test.cc
File src/kudu/util/minidump-test.cc:

PS21, Line 85: #if defined(ADDRESS_SANITIZER)
: // CHECK raises a SIGSEGV under ASAN and fails this test.
: return;
: #endif
This one is also surprising. CHECK() should yield a SIGABRT under the hood, and ASAN converts it to a SIGSEGV? Any interesting ASAN logs to go with it?

Line 113: // ASAN appears to catch SIGBUS, SIGSEGV, and SIGFPE and the process is not killed.
Is there any documentation to corroborate this (and the TSAN case)? It's somewhat surprising; I would expect the sanitizers to at least chain their handlers to ours, otherwise they're breaking signal handling semantics for applications that use them.

Line 117: #elif defined(THREAD_SANITIZER)
I think it's technically possible to build with both ASAN and TSAN, so just for safety maybe convert these into two separate #ifdefs?

While you're at it, you may want to reorder this so that the LOG(INFO) isn't emitted erroneously (i.e. for signals that aren't actually being tested).

http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS6, Line 28: #if defined(__linux__)
: #include <breakpad/client/linux/handler/exception_handler.h>
: #include <breakpad/common/linux/linux_libc_support.h>
: #endif // defined(__linux__)
> It sounds good in theory but then you have to worry about gflag DEFINEs (do
Agreed that #ifdefs just here and conditional compilation of the minidump tests is net better than #ifdefs everywhere.

http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 68: return false; // NOLINT(*)
What does lint dislike about this?

PS21, Line 178: non-user
Nit: non-user signal (or non-user-signal)

Line 179: google::InstallFailureFunction(nullptr);
Maybe we should move this down to just before the breakpad exception handler is actually installed? We wouldn't want this behavior on failure, right?

PS21, Line 261: // Send SIGUSR1 signal to thread, which will wake it up.
: kill(getpid(), SIGUSR1);
I found this confusing, since technically we're sending the signal to the entire process. AFAICT this means it is distributed to any thread in the process at random. But the way this works (I think) is:
- We block SIGUSR1 in the process early, before any threads have started.
- Any new thread inherits the signal mask of its parent, so all Kudu threads have SIGUSR1 blocked.
- A thread calling sigwait() unblocks SIGUSR1 only to itself, guaranteeing that a SIGUSR1 will be picked up by sigwait().

This is based on my reading of http://elias.rhi.hi.is/libc/Threads-and-Signal-Handling.html.

Out of curiosity, did you look into using signalfd() instead? The advantage of signalfd() is that now you have yet another fd to wait on for delivering SIGUSR1, and you can use primitives like select(), poll(), or epoll() to wait on it in the same way you'd wait on other stuff, or wait on it in libev. This requires more boilerplate code, but should allow you to use a real condition variable to notify the thread that it should die vs. sending a SIGUSR1.

I'm nervous enough about UNIX signal handling that I can't recommend one over the other, but just I thought I'd ask. https://ldpreload.com/blog/signalfd-is-useless?reposted-on-request is also a good read, though it's about signalfd(), not sigwait().

Incidentally, a blocked SIGUSR1 will be inherited by all Kudu subprocesses. I don't think that's an issue today because we don't launch subprocesses in production, but we will soon (Todd is introducing something like that for kerberos). Blocking SIGUSR1 for them may be innocuous if they're short-lived, but if they're long-lived it could be problematic. May be a big fat warning somewhere in subprocess.h is in order?

Line 270: CHECK_EQ(0, sigwait(&signals, &signal));
maybe PCHECK() would be more appropriate here, since EINVAL via errno is technically possible?

http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 41: // handling facilities, this class must be invoked after installing those
> I attempted using a GoogleOnce-based approach before rewriting this as a si
I don't follow issue #1. Every KuduTest includes a directory hierarchy that's created at test start and deleted recursively at test stop. It'd be natural for the minidump files to be placed somewhere in that hierarchy and then you'd get automatic cleanup. Why doesn't this work?

I also don't understand #2. Why is it important for different test invocations to use the same minidump directory? Why can't KuduTest provide a method that'll give an individual test direct access to that directory, wherever it may be?

Personally I don't like introducing new singletons because it gets really messy when you need to do something like plumb non-singleton state into the singleton. An example that plagued me in the past: plumbing a memtracker (non-singleton) into the block cache (singleton). That gets messy in a MiniCluster world.

I also think that all our singletons should adhere to the same (narrow) interface: a GetInstance() method that returns a bare pointer to the singleton object, constructing it when calling for the first time. This means there's no lifecycle; the object is created implicitly when it's first needed, and lives forever. It can be easily implemented with a GoogleOnce or what-have-you, and is very easy to reason about.

I think a "singleton with a lifecycle" is the worst of both worlds, IMHO. You've got all the plumbing problems that a singleton gives you, along with all of the complexity of an object lifecycle.

--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(46 comments)

http://gerrit.cloudera.org:8080/#/c/5620/3//COMMIT_MSG
Commit Message:

Line 7: Add Google Breakpad support to Kudu
> What does breakpad support mean for regular core dumps? Are they no longer 
Done


PS3, Line 12: part
> Nit: is part
Done


PS3, Line 17: critical crash information
> Since your audience here is Kudu developers, it would be nice to add a touc
It's well documented; I'll add a link to the docs.


Line 21: the default log directory for a given Kudu daemon process.
> Why was the log directory chosen as a suitable default home?
Added a note to the commit message.


Line 36: 
> It would be good to include a section on how-to go about debugging the mini
I added a link to the breakpad docs, which explain the process. But we should also follow up with formal used documentation about use of breakpad in Kudu for the next release.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

PS3, Line 21: #include <glog/logging.h>
> nit: is it needed?
Not anymore. Removed


Line 21: #include <glog/logging.h>
> nit: consider adding <gtest/gtest.h>
I don't think we need that. We get it from the base class.


PS3, Line 22: #include <glog/stl_logging.h>
> nit: is it needed?
Removed


PS3, Line 24: #include "kudu/gutil/strings/util.h"
> nit: is it needed?
Removed


PS3, Line 26: #include "kudu/util/logging.h"
> nit: is it needed?
Removed


Line 28: #include "kudu/util/subprocess.h"
> nit: consider including "kudu/util/monotime.h" (for MonoDelta)
Done


Line 66: 
> You might want to add a test for writing minidumps on SIGUSR1, especially s
Good idea. Done.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:

PS3, Line 60: argv[0]
> nit: consider adding a var for argv[0] and using it around instead.
Especially since it is read-only, I personally find passing in argv[0] to make the intention clearer.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 1246:       return IOError(Substitute("Unable to canonicalize $0", path), errno);
> Maybe this was intended for the path_util patch? Doesn't really matter thou
It could go either way. It doesn't really belong to anything, it's just a minor improvement.


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 48: DEFINE_bool(enable_minidumps, true, "Whether to enable minidump collection upon process crash");
> Should also tag it with evolving, I think.
Done


Line 50: TAG_FLAG(enable_minidumps, runtime);
> How is this safe for changing at runtime? If the initial value was false, R
Yes, that changed during development. Removed.


PS3, Line 56: nly be written when a daemon exits "
            :     "unexpectedly,
> This is actually not true anymore (but used to be), even for Impala, since 
Thanks for the catch. Done


PS3, Line 57: Set to empty to "
            :     "disable writing minidump files.
> Agreed with Alexey; having two ways to disable minidumps (enable_minidumps=
You guys bring up a good point. I took your advice, Adar, thanks for the suggestion.


PS3, Line 62: Set to 0 to keep all minidump files
> Shouldn't we allow 0 for "delete all outstanding minidumps at startup", and
After discussing this with Lars and due to plans for adding a minidump reaper thread, if we changed to the suggested semantics then enable_minidumps=true with max_minidumps=0 would be nonintuitive and appear broken, since the deleter thread would remove the dumps created via SIGUSR1. So I think it's better to keep this flag the way it is.


Line 76: static google_breakpad::ExceptionHandler* minidump_exception_handler = nullptr;
> Our convention is to prefix global variables with g_.
Done


PS3, Line 84: by breakpad 
> Nit: drop this; it's implied by "Callback for breakpad".
Done


PS3, Line 85: It logs the event before breakpad crashes
            : // the process
> Oh, so breakpad crashes the process now? Previously glog was doing that for
I hadn't considered this, but it turned out to be an issue.

Per IMPALA-3656, Breakpad is supposed to work with glog 0.3.4 but I had to backport a patch from glog master to get CHECK() to both display a stack trace and generate a minidump. I'll have to check with Lars about whether they had to do the same backport.

In any case, was able to address the previous issues with this. Now, we get both a minidump and a stack trace on a CHECK() or a fatal signal.


PS3, Line 97: sizeof(msg) / sizeof(msg[0]) - 1;
> If we're ok calling into my_strlen() for the path, let's use it here too fo
Done


PS3, Line 99: libc
> Would be good to say _why_ we shouldn't call into libc here.
Done


Line 118:   minidump_exception_handler->WriteMinidump(FLAGS_minidump_path, DumpCallback, nullptr);
> You sure this is safe to call from a signal handler? The docs (https://chro
This is the USR1 signal handler. I'll rename it and add some comments to clarify.


Line 137:   Env* env = Env::Default();
> You can get this from the caller.
Done


PS3, Line 139:   // Search for minidumps. There could be multiple minidumps for a single second.
             :   multimap<int, string> timestamp_to_path;
> FWIW, I think the approach used in the glog cleanup thread is nicer: a vect
sgtm, done


Line 147:     unique_ptr<SequentialFile> in;
> I think this is overkill. We don't verify that files that with glog-looking
This code was borrowed from Impala, and I tend to agree with you that simpler is probably better. I reused the logic from the glog cleaner thread.


Line 158:     constexpr int kHeaderSize = sizeof(MDRawHeader);
> So breakpad doesn't namespace its classes either? Would have expected this 
yup. not namespaced.


PS3, Line 160: Slice
> Do we explicitly want to include #include "kudu/util/slice.h" for this ?
Done


PS3, Line 164: Attempting to delete it
> ok that makes sense, perhaps we could just reorder the error as "failed to 
I simplified this whole bit of logic. Now, if we can't delete the excess files on startup, it will be a fatal error. Otherwise, it generates a warning that blocks rotation of additional files.


Line 203:   static bool registered = false;
> Maybe you can use a google once on minidump_exception_handler instead?
I thought about it (this code came from Impala), and we could, but this is process-wide initialization stuff, so we don't need to worry about thread safety.


Line 207:   if (!FLAGS_enable_minidumps || FLAGS_minidump_path.empty()) return Status::OK();
> If you skip registering the signal handler for SIGUSR1 here and someone sen
Thanks for the catch. Fixed.


PS3, Line 215:   if (!env->FileExists(FLAGS_minidump_path)) {
             :     RETURN_NOT_OK(env->CreateDir(FLAGS_minidump_path));
             :   }
> Maybe CreateDirRecursively() here, so that it probably handles symlinks and
I intended to use CreateDirRecursively() here, which is why I wrote it, but Todd had a problem with it in a previous review. I tend to agree with you on this. I'll ask Todd to chime back in here.


PS3, Line 221: Impala
> I think the intent was to explain that this behavior (in kudu/util/minidump
Adar is correct


PS3, Line 225:   // Create the directory if it is not there. The minidump doesn't get written if there is
             :   // no directory.
             :   if (!env->FileExists(FLAGS_minidump_path)) {
             :     RETURN_NOT_OK(env->CreateDir(FLAGS_minidump_path));
             :   }
> Here too? Maybe combine them into one CreateDirRecursively() call, which is
See above; that's how I originally wrote it in CR1


PS3, Line 242: because usually only process crashes will trigger the
             :   // creation of new minidump files (they are also created on SIGUSR1).
> As you say, SIGUSR1 breaks that guarantee. What about doing what we did for
Done


PS3, Line 252: << "."
> Why the period at the end? Seems unusual for a Kudu LOG statement.
Done


PS3, Line 259: desc, FilterCallback, DumpCallback, nullptr, true, -1);
> Would be nice to reformat this so that one arg is on each line, then you ca
Done


http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 28: Status RegisterMinidump(const std::string& cmd_line_path);
> If this is just a way to get the executable's argv[0], you can use gflags' 
Done and done.


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

Line 563:   make -j$PARALLEL install
> Add $EXTRA_MAKEFLAGS too; see above.
Done


Line 573:     xargs perl -p -i -e '@pre = qw(client common google_breakpad processor third_party); for $p (@pre) { s{#include "$p/}{#include "breakpad/$p/}; }'
> Could you add perl to REQUIRED_TOOLS in preflight.py? This is the second us
Done


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

PS3, Line 284: F_ALL
> Agreed; did you mean F_UNINSTRUMENTED here and F_TSAN below?
Yeah, bad rebase. Fixed.


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

Line 244: 
> nit: an extra-line, seems like a random change
Done


http://gerrit.cloudera.org:8080/#/c/5620/3/thirdparty/scripts/make-breakpad-src-archive.sh
File thirdparty/scripts/make-breakpad-src-archive.sh:

Line 37: VERSION=${1:-$(git rev-parse HEAD)}
> Nit: call this REVISION to be consistent with LSS_REVISION (and to make it 
Done


Line 41: git archive --format=tar --prefix=$NAME/ $VERSION | (cd .. && tar xf -)
> Is this a fancy way to copy and rename the tree to another location?
Yeah... we export only checked-in files, without the git metadata, and construct the expected directory structure so we can run the build. Then we tar it up. I'll add some comments.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#6).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process. This was
chosen for the following reasons:

Changes in this patch:

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
21 files changed, 847 insertions(+), 9 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/6
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 118:   minidump_exception_handler->WriteMinidump(FLAGS_minidump_path, DumpCallback, nullptr);
> My initial read was that "compromised context" meant "in a signal handler, 
Hrmm. I was interpreting it the other way, but I think you're right... this would not be async-signal safe. For now what I'll do is remove the SIGUSR1 functionality, and file a JIRA against Impala for doing the same thing. The right way to do it would probably be to have a signal-handling thread.

For the moment I'm going to post an update to this patch including this functionality, just to get a test run, but I'll follow up with another version after that.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#5).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process. This was
chosen for the following reasons:

Changes in this patch:

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
21 files changed, 694 insertions(+), 9 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/5
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#27).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/27
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 27
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#11).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/11
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#35).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/35
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 35
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 34:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/34/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 86:   __attribute__((unused)) std::atomic<bool> user_signal_handler_thread_running_;
> Hmm, I thought this typically goes after the variable declaration, not befo
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 21:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/5620/20/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:

PS20, Line 500:     ls -al $TEST_TMPDIR
              :     find $TEST_TMPDIR
> Maybe just "find $TEST_TMPDIR -ls" would yield the same thing as both of th
Done


http://gerrit.cloudera.org:8080/#/c/5620/21/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 51: import static org.junit.Assert.assertTrue;
> Unused?
Done


http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:

Line 62:   CHECK_OK(BlockSigUSR1()); // Must be called before logging threads started.
> Can we do this within InitGoogleLoggingSafe()? It'd affect a few more proce
Done


http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump-test.cc
File src/kudu/util/minidump-test.cc:

PS21, Line 85: #if defined(ADDRESS_SANITIZER)
             :   // CHECK raises a SIGSEGV under ASAN and fails this test.
             :   return;
             : #endif
> This one is also surprising. CHECK() should yield a SIGABRT under the hood,
I have investigated this a bit and so far I'm not quite sure why it's generating the SIGSEGV. Death tests can be difficult to debug under GDB because of all the forking (follow child and follow parent both resulted in me not being able to break on a SIGSEGV signal... I think I need to break on clone and change the fork-follow-mode setting in the middle of execution to debug it).

I ran strace() to make sure and somewhere in the CHECK it is generating a SIGSEGV. This is the output:

Death test: { while (google::_Check_string* _result = google::Check_EQImpl( google::GetReferenceableValue(1), google::GetReferenceableValue(0), "1" " " "==" " " "0")) google::LogMessageFatal("../../src/kudu/util/minidump-test.cc", 93, google::CheckOpString(_result)).stream(); }
    Result: died but not with expected error.
  Expected: kudu::MinidumpDeathTest_TestCheckStackTraceAndMinidump_Test::TestBody()
Actual msg:
[  DEATH   ] F0123 21:44:57.211793 11054 minidump-test.cc:93] Check failed: 1 == 0 (1 vs. 0)
[  DEATH   ] *** Check failure stack trace: ***
[  DEATH   ] ASAN:DEADLYSIGNAL
[  DEATH   ] =================================================================
[  DEATH   ] ==11054==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x7ffd934add30 sp 0x7ffd934add28 T0)
[  DEATH   ] ==11054==Hint: pc points to the zero page.
[  DEATH   ] ==11054==The signal is caused by a READ memory access.
[  DEATH   ] ==11054==Hint: address points to the zero page.
[  DEATH   ]
[  DEATH   ] AddressSanitizer can not provide additional info.
[  DEATH   ] SUMMARY: AddressSanitizer: SEGV (<unknown module>)
[  DEATH   ] ==11054==ABORTING
[  DEATH   ]


Line 113:   // ASAN appears to catch SIGBUS, SIGSEGV, and SIGFPE and the process is not killed.
> Is there any documentation to corroborate this (and the TSAN case)? It's so
I actually can't find any documentation for it. I took a look at the ASAN source and it installs its own signal handlers while providing a way to override them via ASAN_OPTIONS, I believe (based on looking at IsHandleDeadlySignal() in thirdparty/llvm-3.9.1.src/projects/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc).

I don't know much more than that but IMHO it's not that big of a deal to just disable some tests / signals under ASAN. We maintain some ASAN coverage still, but we're mainly concerned about being able to get minidumps in RELEASE mode anyway.


Line 117: #elif defined(THREAD_SANITIZER)
> I think it's technically possible to build with both ASAN and TSAN, so just
Done and done


http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 68:     return false; // NOLINT(*)
> What does lint dislike about this?
It's the clang tidy bot, and I think since kMinidumpPlatformSupported is a constexpr it doesn't handle this well and sees if (value && false) return false


PS21, Line 178: non-user 
> Nit: non-user signal (or non-user-signal)
Done


Line 179:   google::InstallFailureFunction(nullptr);
> Maybe we should move this down to just before the breakpad exception handle
Done


PS21, Line 261:   // Send SIGUSR1 signal to thread, which will wake it up.
              :   kill(getpid(), SIGUSR1);
> I found this confusing, since technically we're sending the signal to the e
You're correct about how sigwait() works.

I didn't look into using signalfd(), since sigwait() seems to work.

Adding a warning to subprocess.h seems a little out of place, since it is kind of a standalone library.


Line 270:     CHECK_EQ(0, sigwait(&signals, &signal));
> maybe PCHECK() would be more appropriate here, since EINVAL via errno is te
OK. I did something similar since sigwait() doesn't set errno.


http://gerrit.cloudera.org:8080/#/c/5620/6/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 41: // handling facilities, this class must be invoked after installing those
> I don't follow issue #1. Every KuduTest includes a directory hierarchy that
After our in-person conversation I went back to the original use case I had for this and found that if I integrated the handler into ServerBase I didn't need the singleton to access the instance from the log cleanup thread, since ServerBase owns that too. So I was able to get rid of the tricky parts of the singleton implementation (the lock and the weak_ptr) in the latest rev.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 21:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump-test.cc
File src/kudu/util/minidump-test.cc:

PS21, Line 85: #if defined(ADDRESS_SANITIZER)
             :   // CHECK raises a SIGSEGV under ASAN and fails this test.
             :   return;
             : #endif
> I have investigated this a bit and so far I'm not quite sure why it's gener
I finally figured this out. The failure function must not be nullptr and must call abort().


http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS21, Line 261:   // Send SIGUSR1 signal to thread, which will wake it up.
              :   kill(getpid(), SIGUSR1);
> > Incidentally, a blocked SIGUSR1 will be inherited by all Kudu subprocesse
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#16).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
26 files changed, 939 insertions(+), 30 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/16
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.

Dinesh Bhat has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/3/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS3, Line 164: Attempting to delete it
> I disagree -- it's easier for grep purposes to keep individual events descr
ok that makes sense, perhaps we could just reorder the error as "failed to read minidump file. Read error: " << s.ToString() << "Attempting to delete it." so that read error message and reason string are adjacent to each other.


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#15).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/15
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 24: -Code-Review

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5620/21/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

PS21, Line 261:   user_signal_handler_thread_->Join();
              : }
> You're correct about how sigwait() works.
> Incidentally, a blocked SIGUSR1 will be inherited by all Kudu subprocesses. I don't think that's an issue today because we don't launch subprocesses in production, but we will soon (Todd is introducing something like that for kerberos). Blocking SIGUSR1 for them may be innocuous if they're short-lived, but if they're long-lived it could be problematic.

We discussed this on Slack. Apparently exec() doesn't reset signal handlers for block signals[1], and Todd pointed out that the JVM reserves SIGUSR1 for internal use. So if we were to launch a JVM via our subprocess mechanism, it might encounter issues.

So let's unblock (just SIGUSR1? all?) signals after the fork() but before exec() in subprocess.cc.

1. https://lwn.net/Articles/415684/


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#12).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/12
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#8).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the default log directory for a given Kudu daemon process. This was
chosen for the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (modified) util/minidump.{cc,h} from Apache Impala (incubating).
The modifications mostly consist of removing use of boost::filesystem
and conformance to the Kudu code style guidelines.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
A cmake_modules/FindBreakpadClient.cmake
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/master_main.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env_posix.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
22 files changed, 937 insertions(+), 9 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/8
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#23).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/logging.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
28 files changed, 928 insertions(+), 41 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/23
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 23
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5620/1//COMMIT_MSG
Commit Message:

Line 28:   because breakpad does not properly namespace its includes (however it
I'm not sure what you mean by "namespace" here. You don't mean namespace as in C++ namespace, but rather as in a directory prefix?

In that case, it seems odd to me to put them in the $PREFIX/include dir at all, because people are going to be confused how the includes work.

Alternatively, maybe it would be preferably to post-process the installed directory? ie install into the normal prefix and then run something like:

find $PREFIX/include/breakpad -type f | xargs perl -p -i -e 's,^#include "(?!breakpad/),$&breakpad/,'

from a quick inspection of the source it appears like they are self-consistent about using #include "..." for breakpad headers and #include <...> for system headers.


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/integration-tests/minidump_generation-itest.cc
File src/kudu/integration-tests/minidump_generation-itest.cc:

Line 56:   ASSERT_TRUE(found) << "Unable to find minidump after " << timeout.ToString();
could you restructure this test using AssertEventually(...) to be a bit easier to read? eg pseudocode:

AssertEventually([&](){
  vector<string> matches;
  ASSERT_OK(env_->Glob(dir + "/*.dmp", &matches));
  ASSERT_EQ(1, matches.size());
});


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:

Line 208:   breakpad_client
does putting this stuff in util/ mean that we're ending up linking unnecessary junk into our client library? or does the linking process end up not including any of the breakpad stuff because client code never calls it?


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.cc
File src/kudu/util/minidump.cc:

Line 99:     const int path_len = my_strlen(path);
I don't think it's calling into libc that's problematic, is it? I think the real issue is async signal safety of strlen(), though according to http://austin-group-l.opengroup.narkive.com/jBp07fPN/adding-simple-string-functions-to-async-signal-safe-list it is in practice and seems like it has been designated as such in later versions of the standard).

I'm also surprised to see 'my_strlen' is defined by breakpad without any namespacing or anything. yuck.


PS1, Line 220: CreateDirsRecursively
we sure that recursive create is what we want vs just creating the parent and the daemon directory?


http://gerrit.cloudera.org:8080/#/c/5620/1/src/kudu/util/minidump.h
File src/kudu/util/minidump.h:

Line 26: Status RegisterMinidump(const char* cmd_line_path);
seems odd to see a C string here instead of std::string


-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: Add Google Breakpad support to Kudu
......................................................................


Patch Set 28: Verified+1

Overriding Jenkins failure due to a flaky java test.

-- 
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 28
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#31).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/31
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 31
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Mike Percy, Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#34).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master
processes.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.
* Remove unused includes from mini_tablet_server.cc
* Clean up signal handling logic when forking a subprocess.
* Hide breakpad symbols from libkuduclient.so
* Install failure function that simply calls abort() instead of first
printing a stacktrace.

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/34
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add Google Breakpad support to Kudu

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/5620

to look at the new patch set (#22).

Change subject: Add Google Breakpad support to Kudu
......................................................................

Add Google Breakpad support to Kudu

This patch enables Google Breakpad for the kudu-tserver and kudu-master
processes.

Breakpad is part of the crash-reporting infrastructure open-sourced by
Google. It is also used by Mozilla Firefox and Google Chrome.

For more information, see the breakpad getting started docs at
http://chromium.googlesource.com/breakpad/breakpad/+/master/docs/getting_started_with_breakpad.md

By default, on crash, minidump files are written to a subdirectory of
the log directory for a given Kudu daemon process. This was chosen for
the following reasons:

Changes in this patch:

* Add breakpad to thirdparty.
* Add breakpad source archive creation script (Breakpad does not do
releases).
* Add a "hack" to the breakpad thirdparty installation routine to
forcibly add a breakpad/ prefix to the breakpad header files. This
frees us from having to modify our include path when building.
* Pull in (heavily modified) util/minidump.{cc,h} from Apache Impala
(incubating). The modifications consist of removing use of
boost::filesystem, conformance to the Kudu code style guidelines, and
some significant refactoring.
* Enable breakpad support for the kudu-tserver and kudu-master process
startup routines.
* By default, invoke previously-installed signal handler if installed.
* Handle SIGUSR1 by safely using sigwait() to create minidumps on demand.
* Add #ifdefs and CMake logic for Linux.
* Add test for all the deadly signals to ensure we get both a stack
trace and a minidump, except for the case of SIGTERM, where we should
not get a minidump (by design).
* Change a few library unit tests that relied on SIGUSR1 to use another
signal for their testing, such as SIGHUP or SIGUSR2.

Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
---
M CMakeLists.txt
M build-support/jenkins/build-and-test.sh
A cmake_modules/FindBreakpadClient.cmake
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/client/client_samples-test.sh
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
A src/kudu/integration-tests/minidump_generation-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/debug-util-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/logging.cc
A src/kudu/util/minidump-test.cc
A src/kudu/util/minidump.cc
A src/kudu/util/minidump.h
M src/kudu/util/subprocess-test.cc
M src/kudu/util/test_main.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/preflight.py
A thirdparty/scripts/make-breakpad-src-archive.sh
M thirdparty/vars.sh
26 files changed, 914 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5620/22
--
To view, visit http://gerrit.cloudera.org:8080/5620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I495695cc38b75377f20b0497093a81ed5baa887f
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>