[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9250 ) Change subject: IMPALA-6519: API to allocate unreserved buffer .. Patch Set 6: Verified+1 -- 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: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 15 Feb 2018 01:35:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9250 ) Change subject: IMPALA-6519: API to allocate unreserved buffer .. IMPALA-6519: API to allocate unreserved buffer The motivation is to allow allocation of buffers without reservation in ExchangeNode. Currently this it not possible because IncreaseReservationToFit() followed by AllocateBuffer() is non-atomic. We need to handle concurrent allocations in ExchangeNode because there may be multiple batches being received at a given time. This is a temporary solution until we can implement proper reservations in ExchangeNode (IMPALA-6524). Testing: Added basic unit test. Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Reviewed-on: http://gerrit.cloudera.org:8080/9250 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h 6 files changed, 131 insertions(+), 33 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9250 ) Change subject: IMPALA-6519: API to allocate unreserved buffer .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1939/ -- 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: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 21:56:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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: (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: } > That bug probably wouldn't have happened if the code was just put into the Yes I undermined myself a bit there. http://gerrit.cloudera.org:8080/#/c/9250/4/be/src/runtime/bufferpool/buffer-pool.cc@570 PS4, Line 570: if (success != nullptr) *success = false; > I was suggesting just moving the logic directly into BufferPool::AllocateUn I'll stick with this for now. -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 21:56:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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 6: Code-Review+2 -- 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: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 21:56:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 21:48:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 21:29:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Hello Michael Ho, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9250 to look at the new patch set (#5). Change subject: IMPALA-6519: API to allocate unreserved buffer .. IMPALA-6519: API to allocate unreserved buffer The motivation is to allow allocation of buffers without reservation in ExchangeNode. Currently this it not possible because IncreaseReservationToFit() followed by AllocateBuffer() is non-atomic. We need to handle concurrent allocations in ExchangeNode because there may be multiple batches being received at a given time. This is a temporary solution until we can implement proper reservations in ExchangeNode (IMPALA-6524). Testing: Added basic unit test. Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 --- M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h 6 files changed, 131 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/9250/5 -- 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: newpatchset Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9250 ) Change subject: IMPALA-6519: API to allocate unreserved buffer .. Patch Set 4: @tarmstrong. oops. sorry, must have picked the wrong change-id when squashing multiple commits -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 18:19:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 17:31:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
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: @kwho I think you took over my Change-Id for your draft patch. I deleted the draft for now. -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Feb 2018 16:53:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Hello Michael Ho, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9250 to look at the new patch set (#4). Change subject: IMPALA-6519: API to allocate unreserved buffer .. IMPALA-6519: API to allocate unreserved buffer The motivation is to allow allocation of buffers without reservation in ExchangeNode. Currently this it not possible because IncreaseReservationToFit() followed by AllocateBuffer() is non-atomic. We need to handle concurrent allocations in ExchangeNode because there may be multiple batches being received at a given time. Testing: Added basic unit test. Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 --- M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h 6 files changed, 106 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/9250/4 -- 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: newpatchset Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Hello Michael Ho, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9250 to look at the new patch set (#3). Change subject: IMPALA-6519: API to allocate unreserved buffer .. IMPALA-6519: API to allocate unreserved buffer Testing: Added basic unit test. Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 --- M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h 6 files changed, 106 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/9250/3 -- 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: newpatchset Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6519: API to allocate unreserved buffer
Hello Michael Ho, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9250 to look at the new patch set (#2). Change subject: IMPALA-6519: API to allocate unreserved buffer .. IMPALA-6519: API to allocate unreserved buffer Testing: Added basic unit test. Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 --- M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/reservation-tracker.cc M be/src/runtime/bufferpool/reservation-tracker.h 6 files changed, 107 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/9250/2 -- 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: newpatchset Gerrit-Change-Id: Ia4d17b3db25491f796484de22405fbdee7a0f983 Gerrit-Change-Number: 9250 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
