Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12133 )
Change subject: IMPALA-7446: enable buffer pool GC when near process mem limit ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/bufferpool/buffer-pool-test.cc File be/src/runtime/bufferpool/buffer-pool-test.cc: http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/bufferpool/buffer-pool-test.cc@2260 PS4, Line 2260: // Make sure we have a process memory tracker that uses TCMalloc metrics. nit: maybe explain here briefly why those metrics are necessary. Its not obvious unless you read the JIRA or the patch and know which code-path this is trying to hit http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/bufferpool/buffer-pool-test.cc@2277 PS4, Line 2277: ASSERT_OK(client.DecreaseReservationTo(numeric_limits<int64_t>::max(), 0)); nit: should we add a check here to see that the relevant Buffer Pool metrics are in the required state (FREE_BUFFER_BYTES, CLEAN_PAGE_BYTES and UNUSED_RESERVATION_BYTES), to make sure the right code path is hit http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/exec-env.h@254 PS4, Line 254: Must be called after : /// InitBufferPool(). and RegisterMemoryMetrics() http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/12133/4/be/src/runtime/exec-env.cc@436 PS4, Line 436: DCHECK(AggregateMemoryMetrics::TOTAL_USED != nullptr); nit: DCHECK(AggregateMemoryMetrics::TOTAL_USED != nullptr) << "Mem metrics not registered."; -- To view, visit http://gerrit.cloudera.org:8080/12133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81e8e29f1ba319f1b499032f9518d32c511b4b21 Gerrit-Change-Number: 12133 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Comment-Date: Mon, 07 Jan 2019 21:07:24 +0000 Gerrit-HasComments: Yes
