You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2020/04/07 16:46:57 UTC
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15673
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
---
M be/src/runtime/runtime-filter.cc
1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15673/1
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
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/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Add new be test runtime-filter-test.cc
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Reviewed-on: http://gerrit.cloudera.org:8080/15673
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-filter-test.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-filter.h
4 files changed, 133 insertions(+), 12 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5764/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 19:37:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5629/
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 00:59:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15673
to look at the new patch set (#4).
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Add new be test runtime-filter-test.cc
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-filter-test.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-filter.h
4 files changed, 132 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15673/4
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15673/4/be/src/runtime/runtime-filter-test.cc
File be/src/runtime/runtime-filter-test.cc:
http://gerrit.cloudera.org:8080/#/c/15673/4/be/src/runtime/runtime-filter-test.cc@21
PS4, Line 21: #include "common/object-pool.h"
> missed this earlier - common/names.h should go after all the other includes
Done
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 19:12:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 3:
(5 comments)
Thanks for adding the test! We have also done things with "debug actions" that can be enabled with end-to-end tests. E.g. see PUBLISH_FILTER_DELAY. But I think a unit test is better than an end-to-end test for this.
I have a bunch of style comments but the tests and logic LGTM
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc
File be/src/runtime/runtime-filter-test.cc:
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@51
PS3, Line 51: callWaitFor
nit: CallWaitFor() according to the style guide: https://google.github.io/styleguide/cppguide.html#Function_Names
You can also remove the need for the void parameters if you use a C++ thread + lambda.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@52
PS3, Line 52: auto tc = (TestConfig*)vargp;
nit: we use c++-style casts. reinterpret_cast<TestConfig>(...) in this case.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@54
PS3, Line 54: NULL
nit: nullptr
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@82
PS3, Line 82: pthread_create(&t1, NULL, callWaitFor, (void*)&tc);
We use C++-style threads in the codebase which wrap pthreads and provide some niceties. E.g. see BufferTransferConcurrent in buffer-pool-test.cc for an example.
You can use C++ lambdas in conjunction with those, which I think means you can reduce this to something like:
threads.add_thread(new thread([&tc] {
tc.runtime_filter->WaitForArrival(tc.wait_for_ms);
});
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@109
PS3, Line 109: ahead
nit: a head
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 18:07:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15673
to look at the new patch set (#2).
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
---
M be/src/runtime/runtime-filter.cc
1 file changed, 18 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15673/2
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 20:26:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15673
to look at the new patch set (#3).
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Add new be test runtime-filter-test.cc
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-filter-test.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-filter.h
4 files changed, 147 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15673/3
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5735/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 17:05:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 4:
(5 comments)
Thanks, Tim! Learn new stuff today.
In this Patch Set 4, I also wrap delay injection code with NDEBUG macro so that it will be gone in release build.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc
File be/src/runtime/runtime-filter-test.cc:
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@51
PS3, Line 51: t that Runt
> nit: CallWaitFor() according to the style guide: https://google.github.io/s
Replaced with lambda.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@52
PS3, Line 52: // See IMPALA-9612.
> nit: we use c++-style casts. reinterpret_cast<TestConfig>(...) in this case
Gone, replaced with lambda.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@54
PS3, Line 54: eFil
> nit: nullptr
Gone, replaced with lambda.
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@82
PS3, Line 82: MinMaxFilter::Create(ColumnType(PrimitiveType::TYPE_BOOLEAN), &pool_, &tracker_);
> We use C++-style threads in the codebase which wrap pthreads and provide so
Done
http://gerrit.cloudera.org:8080/#/c/15673/3/be/src/runtime/runtime-filter-test.cc@109
PS3, Line 109:
> nit: a head
Done
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 18:53:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 4:
(1 comment)
LGTM aside from a minor nit. I'll +2 and start the merge after that's fixed.
http://gerrit.cloudera.org:8080/#/c/15673/4/be/src/runtime/runtime-filter-test.cc
File be/src/runtime/runtime-filter-test.cc:
http://gerrit.cloudera.org:8080/#/c/15673/4/be/src/runtime/runtime-filter-test.cc@21
PS4, Line 21: #include "common/names.h"
missed this earlier - common/names.h should go after all the other includes in a separate block. It's a special header that imports names into the global namespace based on which headers preceded it (so you want it last).
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 19:04:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:29:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5633/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:29:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15673
to look at the new patch set (#5).
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
In function RuntimeFilter::WaitForArrival, there is a race condition
where condition variable arrival_cv_ may be signaled right after
thread get into the loop and before it call arrival_cv_.WaitFor().
This can cause runtime filter to wait the entire
RUNTIME_FILTER_WAIT_TIME_MS even though the filter has arrived or
canceled earlier than that. This commit avoid the race condition by
making RuntimeFilter::SetFilter and RuntimeFilter::Cancel acquire
arrival_mutex_ first before checking the value of arrival_time_ and
release arrival_mutex_ before signaling arrival_cv_.
Testing:
- Add new be test runtime-filter-test.cc
- Pass core tests.
Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-filter-test.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-filter.h
4 files changed, 133 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/15673/5
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 08:00:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 7:
Looks like a flaky test, I'll file a JIRA.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:29:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 2:
> Patch Set 2:
>
> I should've asked first time, but were you able to reproduce this at all? It'd be good to confirm that it doesn't reproduce after the fix.
>
> Ideally we would add a regression test, but I understand that the regression test may not be feasible with reasonable effort - I didn't see an obvious way to do it without special tooling to force the timing, but maybe you have some ideas.
My first thought is running model checking, but that might be overkill.
We can do simpler by running two threads.
Thread A call RuntimeFilter::WaitForArrival(X).
Thread B call RuntimeFilter::Cancel(), runs slightly after thread A.
We force the race by injecting small delay within WaitForArrival() loop so that Cancel() signal arrival_cv_ before WaitForArrival() arrived at line arrival_cv_.WaitFor().
Racy execution will see test duration >= X ms.
I can reproduce the long waiting bug this way.
Question is, how to correctly inject the time delay so that it only happen in test and not in production?
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 02:48:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5740/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 20:33:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5629/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 20:26:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 7:
Looks like IMPALA-9596
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Apr 2020 03:30:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5760/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 19:29:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15673/1/be/src/runtime/runtime-filter.cc
File be/src/runtime/runtime-filter.cc:
http://gerrit.cloudera.org:8080/#/c/15673/1/be/src/runtime/runtime-filter.cc@31
PS1, Line 31: unique_lock<mutex> l(arrival_mutex_);
nit: we generally use braces to denote the critical section instead of calling unlock() explicitly. I.e. lines 30-40 should be enclosed in a new set of braces and the unlock() call can be removed. The current code is readable enough, just the consistency is preferable.
Also below.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 19:06:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5756/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 16:36:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 2:
I should've asked first time, but were you able to reproduce this at all? It'd be good to confirm that it doesn't reproduce after the fix.
Ideally we would add a regression test, but I understand that the regression test may not be feasible with reasonable effort - I didn't see an obvious way to do it without special tooling to force the timing, but maybe you have some ideas.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 00:15:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 3:
Patch Set 3 add new be test, runtime-filter-test.cc.
This test force the race condition that is happen in IMPALA-9612.
To do so, it instrument RuntimeFilter code to inject delay in the
critical section of function WaitForArrival.
The assertion is that arrival_time_ should be >= injected delay,
because WaitForArrival should maintain mutual exclusion, and
RuntimeFilter should stop waiting once Cancel or SetFilter pass through.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 15:57:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Apr 2020 20:26:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15673 )
Change subject: IMPALA-9612: Fix race condition in RuntimeFilter::WaitForArrival
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15673/1/be/src/runtime/runtime-filter.cc
File be/src/runtime/runtime-filter.cc:
http://gerrit.cloudera.org:8080/#/c/15673/1/be/src/runtime/runtime-filter.cc@31
PS1, Line 31: DCHECK(!HasFilter()) << "SetFilter() should not be called multiple times.";
> nit: we generally use braces to denote the critical section instead of call
Done
Move the DCHECKs within critical section as well, just to be safe.
--
To view, visit http://gerrit.cloudera.org:8080/15673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7dffa626103ef0af06ad1e89231b0d2ee54bb94a
Gerrit-Change-Number: 15673
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 19:55:10 +0000
Gerrit-HasComments: Yes