You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/05/07 16:31:26 UTC
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10314
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
IMPALA-6970: race with decreasing scanner reservation
The usage pattern of DecreaseReservationTo() in
ReturnReservationFromScannerThread() can decrease the reservation by
more than 'bytes' if there is a concurrent IncreaseReservation() call.
The API is changed to fit the requirements of the scanner, which
does not want to release more than the memory reserved for that
scanner thread.
Testing:
Added some backend tests (including concurrency) for the new API.
Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
---
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node.cc
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
8 files changed, 76 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/10314/2
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 3: Code-Review+1
carry +1
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 23:09:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-test.cc
File be/src/runtime/bufferpool/buffer-pool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-test.cc@2158
PS2, Line 2158: thread_group
nit: maybe just use 'thread' if we need only one
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h@349
PS2, Line 349: the postcondition
: /// does not hold if called concurrently with a DecreaseReservation*() call.
do you mean that the post-condition of "after which 'bytes' can be used" does not hold in this case?
if yes, wont this method be confusing to use if its not serving its purpose after returning an OK status.
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 22:19:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 May 2018 03:04:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-internal.h@256
PS2, Line 256: Status DecreaseReservationTo(int64_t max_decrease, int64_t target_bytes) WARN_UNUSED_RESULT;
long line
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 16:32:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h@349
PS2, Line 349: the postcondition
: /// does not hold if called concurrently with a DecreaseReservation*() call.
> do you mean that the post-condition of "after which 'bytes' can be used" do
Maybe just delete that sentence? It seems obvious that if you do Increase and Decrease without some external synchronization then you may not have 'bytes' at the end. Or is it trying to say else?
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 22:31:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 23:10:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 23:24:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10314
to look at the new patch set (#3).
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
IMPALA-6970: race with decreasing scanner reservation
The usage pattern of DecreaseReservationTo() in
ReturnReservationFromScannerThread() can decrease the reservation by
more than 'bytes' if there is a concurrent IncreaseReservation() call.
The API is changed to fit the requirements of the scanner, which
does not want to release more than the memory reserved for that
scanner thread.
Testing:
Added some backend tests (including concurrency) for the new API.
Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
---
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node.cc
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
8 files changed, 74 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/10314/3
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-test.cc
File be/src/runtime/bufferpool/buffer-pool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool-test.cc@2158
PS2, Line 2158: thread incre
> nit: maybe just use 'thread' if we need only one
Done
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:
http://gerrit.cloudera.org:8080/#/c/10314/2/be/src/runtime/bufferpool/buffer-pool.h@349
PS2, Line 349:
: bool IncreaseReservationToFit(int64_t bytes) WARN_UNUSED_RESULT;
> Maybe just delete that sentence? It seems obvious that if you do Increase a
Removed. Yeah it really is obvious :).
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 23:08:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
IMPALA-6970: race with decreasing scanner reservation
The usage pattern of DecreaseReservationTo() in
ReturnReservationFromScannerThread() can decrease the reservation by
more than 'bytes' if there is a concurrent IncreaseReservation() call.
The API is changed to fit the requirements of the scanner, which
does not want to release more than the memory reserved for that
scanner thread.
Testing:
Added some backend tests (including concurrency) for the new API.
Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Reviewed-on: http://gerrit.cloudera.org:8080/10314
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/benchmarks/bloom-filter-benchmark.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node.cc
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
8 files changed, 74 insertions(+), 35 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6970: race with decreasing scanner reservation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10314 )
Change subject: IMPALA-6970: race with decreasing scanner reservation
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2420/
--
To view, visit http://gerrit.cloudera.org:8080/10314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I653225c981bf674d2b2b947f3a3cb4d8f382d36b
Gerrit-Change-Number: 10314
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 May 2018 23:24:24 +0000
Gerrit-HasComments: No