Todd Lipcon 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?
seemed easier than making it a class member, and this isn't perf critical 
enough that the seed computation matters much
PS2, Line 182:   __builtin_unreachable();
> Fancy. If you're feeling generous, there are quite a few places in the code
I suppose it coudl be. This isn't the first use of it, though. It's not 
supported by MSVC but I think we have so many Linux-isms that this is the least 
of our problems :) FWIW Clang on windows is also now pretty feasible (Chrome 
just switched to it)
PS2, Line 204:     } else if (MonoTime::Now() > next_log) {
> >= is more correct, no?
PS2, Line 208:       wakeups.emplace(ComputeNextWakeup(what), what);
> what what what
Ack ack ack
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?
yea, at first I tried to maintain it, but then it just added complexity and I 
couldn't really see the value relative to the extra 5-10 lines of code

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-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Thu, 08 Mar 2018 03:49:37 +0000
Gerrit-HasComments: Yes

Reply via email to