Adar Dembo 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. 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? 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? 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 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)? 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? 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 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. 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? -- 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-Comment-Date: Wed, 07 Mar 2018 20:22:48 +0000 Gerrit-HasComments: Yes
