Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11350 )
Change subject: IMPALA-7446: enable missing buffer pool GC ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/11350/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11350/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-7445 > is this the right JIRA? thanks for checking http://gerrit.cloudera.org:8080/#/c/11350/2//COMMIT_MSG@14 PS2, Line 14: new, soft, : mem_limit > nit: commas? Done http://gerrit.cloudera.org:8080/#/c/11350/2/be/src/runtime/mem-tracker-test.cc File be/src/runtime/mem-tracker-test.cc: http://gerrit.cloudera.org:8080/#/c/11350/2/be/src/runtime/mem-tracker-test.cc@384 PS2, Line 384: Most of the GC attempts should fail > nit: a bit confusing to say GC attempts fail since all GC attempts fail to Done http://gerrit.cloudera.org:8080/#/c/11350/2/be/src/runtime/mem-tracker-test.cc@386 PS2, Line 386: EXPECT_FALSE(t.TryConsume(1, MemLimit::SOFT)); > maybe add a 1ms sleep to ensure some on the gc attemps actually run Is that necessary for the test? Not sure it's worth slowing down the test runtime. We do test the alternative code path above. -- To view, visit http://gerrit.cloudera.org:8080/11350 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32400bda18a36f60cbe315fae715748d33677c10 Gerrit-Change-Number: 11350 Gerrit-PatchSet: 2 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: Thu, 30 Aug 2018 00:03:15 +0000 Gerrit-HasComments: Yes
