Todd Lipcon has posted comments on this change. Change subject: Simplify MemTracker and move process throttling elsewhere ......................................................................
Patch Set 3: (18 comments) http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: PS3, Line 2254: since we can get accurate process memory : // usage statistic > I presume you tested against the kNumOps of 10000, and this new value made yea this magic number seemed to pass http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/server/default-path-handlers.cc File src/kudu/server/default-path-handlers.cc: Line 44: #include "kudu/util/process_memory.h" > warning: #includes are not sorted properly [llvm-include-order] Done Line 47: using boost::replace_all; > warning: using decl 'replace_all' is unused [misc-unused-using-decls] Done Line 148: static void MemTrackersHandler(const Webserver::WebRequest& req, std::ostringstream* output) { > warning: parameter 'req' is unused [misc-unused-parameters] Done Line 149: *output << "<h1>Total memory usage</h1>\n"; > "Total" suggests that the per-subsystem stuff should add up to it. Perhaps Done Line 152: HumanReadableNumBytes::ToString(process_memory::CurrentConsumption())); > Maybe add a warning if !TCMALLOC_ENABLED that this isn't accurate? Done http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/tablet/multi_column_writer.cc File src/kudu/tablet/multi_column_writer.cc: Line 89: LOG(INFO) << "Opened CFile writers for " << cfile_writers_.size() << " column(s)"; > Heh, got tired of this output? yep :) http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.cc File src/kudu/util/mem_tracker.cc: PS3, Line 229: // TODO: This might leave us with an allocated resource that we can't use. Do we need : // to adjust the consumption of the query tracker to stop the resource from never : // getting used by a subsequent TryConsume()? > Probably irrelevant to us. Done Line 240: Consume(-bytes); > Technically this can fail, yet we drop the failure on the ground. I don't think Consume() can fail (only TryConsume fails). Consume always succeeds, right? Line 251: process_memory::MaybeGCAfterRelease(bytes); > Maybe this new bit should be documented in the header somewhere? it's not new, but added it to the header. Line 264: return CheckLimitExceeded(); > Could we just remove one of these two variants if they're identical? Done http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.h File src/kudu/util/mem_tracker.h: PS3, Line 149: LogUsage() > Not relevant anymore. Plus, 'id' is actually used for more than just cosmet Done http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/process_memory.cc File src/kudu/util/process_memory.cc: Line 20: #include <sys/resource.h> > Nit: this should be in its own group ahead of gflags/gperftools since it's Done Line 76: static const int64_t GC_RELEASE_SIZE = 128 * 1024L * 1024L; > warning: 'GC_RELEASE_SIZE' is a static definition in anonymous namespace; s Done Line 141: // Nothing to do if not using tcmalloc. > Maybe this should be moved up into MaybeGCAfterRelease() so we can avoid th Done http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/process_memory.h File src/kudu/util/process_memory.h: Line 30: void MaybeGCAfterRelease(int64_t released_bytes); > error: unknown type name 'int64_t' [clang-diagnostic-error] Done Line 33: int64_t CurrentConsumption(); > error: unknown type name 'int64_t' [clang-diagnostic-error] Done Line 36: int64_t HardLimit(); > error: unknown type name 'int64_t' [clang-diagnostic-error] Done -- To view, visit http://gerrit.cloudera.org:8080/6620 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
