Tim Armstrong 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 ob I expanded a little bit. 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 metric I think it's sufficient to check that SystemBytesAllocated() is 0, since that implies that the other values are 0. I also just called the method directly instead of using the metrics. 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() Done 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 Done -- 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 08 Jan 2019 20:14:26 +0000 Gerrit-HasComments: Yes
