Adar Dembo has posted comments on this change. ( )

Change subject: KUDU-2297 (part 6): enable periodic stack sampling by default, 
avoid bias

Patch Set 2:

File src/kudu/server/diagnostics_log.h:
PS2, Line 59:   enum WakeupType {
enum class is the new hotness.
File src/kudu/server/
PS2, Line 75: If this is set to "
            :              "a non-positive number, stack traces will be not be 
periodically logged.
How about making this a uint and using 0 to disable?
PS2, Line 169:         Random rng(GetRandomSeed32());
Why create a new PRNG each time?
PS2, Line 182:   __builtin_unreachable();
Fancy. If you're feeling generous, there are quite a few places in the codebase 
where we add a comment like // unreachable instead of actually using this, and 
they could be fixed up.

Also, should this be abstracted away by something in gutil/port.h? Or do you 
expect it to be the same everywhere, including in compilers we might not 
support yet (like MSVC)?
PS2, Line 204:     } else if (MonoTime::Now() > next_log) {
>= is more correct, no?
PS2, Line 208:       wakeups.emplace(ComputeNextWakeup(what), what);
what what what
PS2, Line 214:     // Unlock the mutex while actually logging metrics or stacks 
since it's somewhat
This is much cleaner, thanks for the cleanup.
PS2, Line 222:       WARN_NOT_OK(LogStacks(reason), "Unable to collect stacks 
to diagnostics log");
So error no longer feds into the next logging time? Didn't find it useful?

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I538eb59f95a695fa2da50f24ed82148715b15e44
Gerrit-Change-Number: 9523
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Comment-Date: Wed, 07 Mar 2018 20:22:48 +0000
Gerrit-HasComments: Yes

Reply via email to