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 5: Code-Review+2

(2 comments)

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@569
PS4, Line 569:     DCHECK(success != nullptr);
> There was a bug here because we didn't test the failure case. Extended the
That bug probably wouldn't have happened if the code was just put into the 
callers :)


http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc@570
PS4, Line 570:     // The client may not have reserved the memory.
> I had trouble judging whether it was better to have one function with more
I was suggesting just moving the logic directly into 
BufferPool::AllocateUnreservedBuffer and BufferPool::AllocateBuffer. Or can 
they not access the required impl_ fields? Given that they don't really share 
much other than what's under the lock (and have two control flow paramters - 
reserved & success), seems clearer to just have this code inlined.

But if you prefer to leave it the way it is now, that's okay too.



--
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: 5
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 21:48:50 +0000
Gerrit-HasComments: Yes

Reply via email to