Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9250 )
Change subject: IMPALA-6519: API to allocate unreserved buffer ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/9250/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9250/4//COMMIT_MSG@14 PS4, Line 14: let's note that this is a temporary solution so that we can introduce real reservation accounting in the exchange node as a second step. http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.h@246 PS4, Line 246: /// operations for 'client', except for operations on the same 'handle'. maybe note that if there is an unexpected runtime failure during allocation, the reservation may still have been increased? http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.h@250 PS4, Line 250: /// out of that instead of relying on the "best effort" interface. how about explicitly saying that the interface is provided so to help transition components to the buffer pool so that they can implement reservation accounting as a second step during that transition? http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc@562 PS4, Line 562: Ensure we have the reservation required first. that's not really the case when 'reserved' is true -- the client needs to ensure that. Maybe move that first sentence into the else and int he first case say: the client manages the reservation and must ensure it is available, or something? http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc@570 PS4, Line 570: if (success != nullptr) *success = false; given that there's only one callsite with reserved==true and one with reserved==false, maybe refactor this to move the reservation accounting into the callers. Actually, maybe the whole thing should just go into the callers since there's just a couple of lines of common code here. -- To view, visit http://gerrit.cloudera.org:8080/9250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 14 Feb 2018 17:31:55 +0000 Gerrit-HasComments: Yes
