You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/03/28 00:05:06 UTC

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger 

Hello Adar Dembo,

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

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

to review the following change.


Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................

KUDU-2384. Don't collect thread stacks when running under debugger

This changes the stack watchdog and the 'collect all stacks' utility
code to not run when it appears that the process is running under a
tracer such as gdb or strace. This makes debugging Kudu processes
significantly easier because you don't need to know the magical
incantation 'handle SIGUSR2 nostop noprint'.

I tested manually by running a master with
--diagnostics-log-stack-traces-interval-ms=10 and attaching to it with
gdb. With this patch, I didn't see any signals sent.

Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
---
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
5 files changed, 52 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................

KUDU-2384. Don't collect thread stacks when running under debugger

This changes the stack watchdog and the 'collect all stacks' utility
code to not run when it appears that the process is running under a
tracer such as gdb or strace. This makes debugging Kudu processes
significantly easier because you don't need to know the magical
incantation 'handle SIGUSR2 nostop noprint'.

I tested manually by running a master with
--diagnostics-log-stack-traces-interval-ms=10 and attaching to it with
gdb. With this patch, I didn't see any signals sent.

This patch also changes the Env EIO fault injection to never inject
failures on /proc/. This is to avoid a TSAN warning since
KernelStackWatchdog would otherwise access the env_inject_eio_globs flag
concurrent to tests modifying it.

Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
---
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
M src/kudu/util/env_posix.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
6 files changed, 82 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 21:23:27 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3:

This is an interesting case. fs_manager-test is failing because KernelStackWatchdog continues to run in the background after the gflags destructor runs, and now KernelStackWatchdog calls Env::ReadFileToString which checks the fault injection string flag, and that string has been destructed.

So, it seems the potential fixes are:
(a) make that test case reset fault injection in its destructor
(b) make KernelStackWatchdog avoid using Env calls that are subject to fault injection
(c) make fault injection short circuit to "not enabled" for /proc reads (which perhaps is more accurate)

Thoughts?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 01:32:28 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc
File src/kudu/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc@728
PS1, Line 728:   if (IsBeingDebugged()) {
> TriggerAsync is called for each thread. Even though reading /proc/self/stat
Hmm, okay. Could you doc TriggerAsync (or DumpThreadStack, or GetThreadStack) with something like "it's the caller's responsibility to skip this if we're running in a debugger"?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:38:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3:

> > That said, the test in question (TestCreateWithFailedDirs) already resets env_inject_eio to 0 when it finishes; if it didn't, we'd also see a warning that we couldn't clean up the test's temp dir. But I guess here the issue isn't with env_inject_eio, but with env_inject_eio_globs? We don't typically reset that one.
> 
> Yea, the issue is with env_inject_eio_globs which is a string and thus particularly unsafe to read. The odd thing, though, is that we only read env_inject_eio_globs when env_inject_eio is non-zero, but it appears we're still reading it. So perhaps we aren't resetting env_inject_eio properly in all cases or somesuch.

I think it's a real data race: right before the main thread (T1) resets env_inject_eio, the kernel stack watchdog thread (T2) gets scheduled and reads the non-zero gflag value. Then T1 is rescheduled and resets to 0, but it's too late: when T2 is rescheduled, it will read env_inject_eio_globs.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 16:51:06 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................

KUDU-2384. Don't collect thread stacks when running under debugger

This changes the stack watchdog and the 'collect all stacks' utility
code to not run when it appears that the process is running under a
tracer such as gdb or strace. This makes debugging Kudu processes
significantly easier because you don't need to know the magical
incantation 'handle SIGUSR2 nostop noprint'.

I tested manually by running a master with
--diagnostics-log-stack-traces-interval-ms=10 and attaching to it with
gdb. With this patch, I didn't see any signals sent.

This patch also changes the Env EIO fault injection to never inject
failures on /proc/. This is to avoid a TSAN warning since
KernelStackWatchdog would otherwise access the env_inject_eio_globs flag
concurrent to tests modifying it.

Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Reviewed-on: http://gerrit.cloudera.org:8080/9835
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
M src/kudu/util/env_posix.cc
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
6 files changed, 82 insertions(+), 7 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3:

> This is an interesting case. fs_manager-test is failing because KernelStackWatchdog continues to run in the background after the gflags destructor runs, and now KernelStackWatchdog calls Env::ReadFileToString which checks the fault injection string flag, and that string has been destructed.
> 
> So, it seems the potential fixes are:
> (a) make that test case reset fault injection in its destructor
> (b) make KernelStackWatchdog avoid using Env calls that are subject to fault injection
> (c) make fault injection short circuit to "not enabled" for /proc reads (which perhaps is more accurate)
> 
> Thoughts?

I'd be in favor of (c). (a) is unintuitive, and can easily happen again in other tests that turn on fault injection.

That said, the test in question (TestCreateWithFailedDirs) already resets env_inject_eio to 0 when it finishes; if it didn't, we'd also see a warning that we couldn't clean up the test's temp dir. But I guess here the issue isn't with env_inject_eio, but with env_inject_eio_globs? We don't typically reset that one.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 02:19:19 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3:

> Patch Set 3:
> That said, the test in question (TestCreateWithFailedDirs) already resets env_inject_eio to 0 when it finishes; if it didn't, we'd also see a warning that we couldn't clean up the test's temp dir. But I guess here the issue isn't with env_inject_eio, but with env_inject_eio_globs? We don't typically reset that one.

Yea, the issue is with env_inject_eio_globs which is a string and thus particularly unsafe to read. The odd thing, though, is that we only read env_inject_eio_globs when env_inject_eio is non-zero, but it appears we're still reading it. So perhaps we aren't resetting env_inject_eio properly in all cases or somesuch.

Anyway I'll do option (c) since I think it makes sense anyway - we don't want to inject failures reading from proc in general.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 16:37:19 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3:

OK, new revision avoids reading the flags for /proc/ files


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 21:18:11 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc
File src/kudu/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc@728
PS1, Line 728:   if (IsBeingDebugged()) {
Why do this here and not inside StackTraceCollector::TriggerAsync? Between KernelStackWatchdog and SnapshotAllStacks I think you've covered all of the entry points into TriggerAsync, but that can change in the future; better to future-proof it by checking as deeply as possible, no?


http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/os-util.cc
File src/kudu/util/os-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/os-util.cc@166
PS1, Line 166:  = -1
Nit: this initialization is unnecessary, right?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 28 Mar 2018 00:25:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 29 Mar 2018 21:26:32 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................

KUDU-2384. Don't collect thread stacks when running under debugger

This changes the stack watchdog and the 'collect all stacks' utility
code to not run when it appears that the process is running under a
tracer such as gdb or strace. This makes debugging Kudu processes
significantly easier because you don't need to know the magical
incantation 'handle SIGUSR2 nostop noprint'.

I tested manually by running a master with
--diagnostics-log-stack-traces-interval-ms=10 and attaching to it with
gdb. With this patch, I didn't see any signals sent.

Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
---
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
5 files changed, 67 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc
File src/kudu/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc@728
PS1, Line 728:   if (IsBeingDebugged()) {
> Why do this here and not inside StackTraceCollector::TriggerAsync? Between 
TriggerAsync is called for each thread. Even though reading /proc/self/status is quick, I'd rather not have to re-read it hundreds of times in a row.

For reference, on el6 I got:
In [1]: %timeit file("/proc/self/status").read()
100000 loops, best of 3: 12.6 µs per loop

On el7 I got:
In [3]: %timeit file("/proc/self/status").read()
10000 loops, best of 3: 28.7 µs per loop

On my Ubuntu laptop I got:
In [2]: %timeit file("/proc/self/status").read()
10000 loops, best of 3: 38.8 µs per loop

(it appears to be getting slower as more info gets added to it in new kernel versions).

Anyway, with 1000 threads, 38us/loop makes 38ms of overhead, which is a significant enough "skid" for use cases like /stacks that makes it less useful (eg more likely to see 'impossible" situations where two threads are holding the same lock due to the skew between the time when the stacks were collected)


http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/os-util.cc
File src/kudu/util/os-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/os-util.cc@166
PS1, Line 166:  = -1
> Nit: this initialization is unnecessary, right?
yea but it seems that gcc likes to give warnings about use of uninitialized variables in the case that a var is initialized by out-param as is the case here.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:03:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................

KUDU-2384. Don't collect thread stacks when running under debugger

This changes the stack watchdog and the 'collect all stacks' utility
code to not run when it appears that the process is running under a
tracer such as gdb or strace. This makes debugging Kudu processes
significantly easier because you don't need to know the magical
incantation 'handle SIGUSR2 nostop noprint'.

I tested manually by running a master with
--diagnostics-log-stack-traces-interval-ms=10 and attaching to it with
gdb. With this patch, I didn't see any signals sent.

Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
---
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
M src/kudu/util/kernel_stack_watchdog.cc
M src/kudu/util/os-util.cc
M src/kudu/util/os-util.h
5 files changed, 68 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc
File src/kudu/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/9835/1/src/kudu/util/debug-util.cc@728
PS1, Line 728:   if (IsBeingDebugged()) {
> Hmm, okay. Could you doc TriggerAsync (or DumpThreadStack, or GetThreadStac
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:50:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2384. Don't collect thread stacks when running under debugger

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9835 )

Change subject: KUDU-2384. Don't collect thread stacks when running under debugger
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib17ffb9514e9b440b8d97843374d2a60913a398b
Gerrit-Change-Number: 9835
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:52:38 +0000
Gerrit-HasComments: No