Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11103 )
Change subject: IMPALA-7096: restore scanner thread memory heuristics ...................................................................... Patch Set 7: Code-Review+2 (6 comments) http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h File be/src/runtime/scanner-mem-limiter.h: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@32 PS7, Line 32: limiting the number of scanner threads is it to limit the number of scanner threads, or the aggregate memory consumption by scanner threads? http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@42 PS7, Line 42: as long until garbled http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@43 PS7, Line 43: tears down all control structures. is that requirement because the instance of this class happens to also be a QueryState control structure? If so, maybe clearer to just say node must outlive the lifetime of this object? (i.e. to indicate that this object is gonna keep a reference to it). Since, it's not really dictated by this abstraction who owns the instance of '*this'. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.h@71 PS7, Line 71: std::vector<std::unique_ptr<RegisteredScan>> registered_scans_; do we need that? why not just iterate over the map? do we want ordering but don't want to use map<>? http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc File be/src/runtime/scanner-mem-limiter.cc: http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@75 PS7, Line 75: This is carried over from old versions of the : // code. what code? before this change or before the change that removed the original heuristic? only because this is so arbitrary, it might help be be more specific in this reference. http://gerrit.cloudera.org:8080/#/c/11103/7/be/src/runtime/scanner-mem-limiter.cc@81 PS7, Line 81: // Add the expected increase in consumption for existing threads. : addtl_consumption += static_cast<int64_t>(consumption * 0.5); I don't really understand that. Why would adding this thread increase consumption of the other threads? oh, maybe this is just saying that we're guessing in the future the threads may happen to grow by 50% over their current usage? -- To view, visit http://gerrit.cloudera.org:8080/11103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9907fa8c4d2b0b85f67f4f160899c1c258ad82b Gerrit-Change-Number: 11103 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 15 Aug 2018 18:03:26 +0000 Gerrit-HasComments: Yes
