Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9523 )
Change subject: KUDU-2297 (part 6): enable periodic stack sampling by default, avoid bias ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.h File src/kudu/server/diagnostics_log.h: http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.h@59 PS2, Line 59: enum WakeupType { > enum class is the new hotness. Done http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@75 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? Done http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@169 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 http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@182 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) http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@204 PS2, Line 204: } else if (MonoTime::Now() > next_log) { > >= is more correct, no? Done http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@208 PS2, Line 208: wakeups.emplace(ComputeNextWakeup(what), what); > what what what Ack ack ack http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@214 PS2, Line 214: // Unlock the mutex while actually logging metrics or stacks since it's somewhat > This is much cleaner, thanks for the cleanup. Ack http://gerrit.cloudera.org:8080/#/c/9523/2/src/kudu/server/diagnostics_log.cc@222 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 http://gerrit.cloudera.org:8080/9523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I538eb59f95a695fa2da50f24ed82148715b15e44 Gerrit-Change-Number: 9523 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 03:49:37 +0000 Gerrit-HasComments: Yes
