Adar Dembo has posted comments on this change. Change subject: KUDU-1410 (part 1). Add per-Trace counters ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/2794/3//COMMIT_MSG Commit Message: Line 37: Why not also counters for disk writes? Is it because they're harder to associate back to their requests? http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 994: TRACE_COUNTER_INCREMENT("lbm_read_time_us", dur); No love for the file block manager? http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/util/mutex.cc File src/kudu/util/mutex.cc: Line 73: // Optimize for the case when mutexes are uncontended. If they : // are contended, we'll have to go to sleep anyway, so the extra : // cost of branch mispredictions is moot. : if (PREDICT_TRUE(TryAcquire())) { : return; : } Seems unrelated to the trace metrics change. Or is this because you didn't want to restrict measuring wait time to debugging builds? Does TryAcquire() make a system call? I poked around the glibc source but couldn't find out definitively. If it does, I'd argue that a contended Acquire() just doubled in expense. http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: Line 64: // unbounded cardinality (a server may have thousands of differnet tablet IDs Nit: different Line 65: In that case, set the prefix to "raft" to avoid issues. It took me several readings to understand that if trace_metric_prefix is NOT set, the name is used as the prefix. Maybe find a way to state that more explicitly? http://gerrit.cloudera.org:8080/#/c/2794/3/src/kudu/util/trace_metrics.h File src/kudu/util/trace_metrics.h: Line 17: #ifndef KUDU_UTIL_TRACE_METRICS_H : #define KUDU_UTIL_TRACE_METRICS_H Let's start using "#pragma once" now that we're relying on C++11. Line 48: static const char* InternName(const std::string& name); The name is never generated using user data, right? Asking because only encodings whose strings are nul-terminated are allowed (due to the use of c_str()), and user data could be pretty much anything. -- To view, visit http://gerrit.cloudera.org:8080/2794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86001778976f01b62342f63f98b62962bc74212f Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
