Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9250 )

Change subject: IMPALA-6519: API to allocate unreserved buffer
......................................................................


Patch Set 4:

(6 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
Done


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
Done


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 trans
Done


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 e
Restructured the comments to separate the two cases.


http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc@569
PS4, Line 569:   }
There was a bug here because we didn't test the failure case. Extended the test 
to exercise it.


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 reser
I had trouble judging whether it was better to have one function with more 
complex control flow vs two functions with  simpler control flow. Getting the 
accounting right was a little subtle so I thought this way we'd get better test 
coverage, but I can see the argument for the other approach if you think it is 
superior.



--
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 21:29:25 +0000
Gerrit-HasComments: Yes

Reply via email to