You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/10/27 05:11:41 UTC
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4864
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
IMPALA-4383: Ensure plan fragment report thread is always started
PlanFragmentExecutor::report_thread_active_ was set during
OpenInternal() after ReportProfile() - run in a different thread -
signalled that it was running.
Howeer, ReportProfile() reads report_thread_active_ to determine if it
should exit its loop. If it runs fast enough, ReportProfile() will never
see report_thread_active_ == true.
This patch moves setting report_thread_active_ to ReportProfile() -
slightly earlier, but visible at almost the same time because it is now
correctly protected by report_thread_lock_.
Testing:
* TestRPCTimeout would hit this in certain configurations. Ran
test_execplanfragment_timeout() for 90 minutes with no failures;
previously it would fail within the first five attempts repeatedly.
Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
---
M be/src/runtime/plan-fragment-executor.cc
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/4864/1
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4864/1/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:
Line 169: bool report_thread_active_; // true if we started the thread
> The protocol could do with some documentation - I had to piece it together
Done
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4864
to look at the new patch set (#2).
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
IMPALA-4383: Ensure plan fragment report thread is always started
PlanFragmentExecutor::report_thread_active_ was set during
OpenInternal() after ReportProfile() - run in a different thread -
signalled that it was running.
Howeer, ReportProfile() reads report_thread_active_ to determine if it
should exit its loop. If it runs fast enough, ReportProfile() will never
see report_thread_active_ == true.
This patch moves setting report_thread_active_ to ReportProfile() -
slightly earlier, but visible at almost the same time because it is now
correctly protected by report_thread_lock_.
Testing:
* TestRPCTimeout would hit this in certain configurations. Ran
test_execplanfragment_timeout() for 90 minutes with no failures;
previously it would fail within the first five attempts repeatedly.
Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
---
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
2 files changed, 6 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/4864/2
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 1:
FragmentComplete() might not be called if the fragment never completes (e.g. perhaps it's waiting for input from another instance that's been cancelled). So I don't think we can guarantee that that path gets taken in all executions.
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4864/1/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:
Line 169: bool report_thread_active_; // true if we started the thread
The protocol could do with some documentation - I had to piece it together by reading the code.
E.g. - when the report thread starts, it sets 'report_thread_active_' to true and signals 'report_thread_started_cv_'. The report thread is shut down by setting 'report_thread_active_' to false and signalling 'stop_report_thread_cv_'.
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
IMPALA-4383: Ensure plan fragment report thread is always started
PlanFragmentExecutor::report_thread_active_ was set during
OpenInternal() after ReportProfile() - run in a different thread -
signalled that it was running.
Howeer, ReportProfile() reads report_thread_active_ to determine if it
should exit its loop. If it runs fast enough, ReportProfile() will never
see report_thread_active_ == true.
This patch moves setting report_thread_active_ to ReportProfile() -
slightly earlier, but visible at almost the same time because it is now
correctly protected by report_thread_lock_.
Testing:
* TestRPCTimeout would hit this in certain configurations. Ran
test_execplanfragment_timeout() for 90 minutes with no failures;
previously it would fail within the first five attempts repeatedly.
Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Reviewed-on: http://gerrit.cloudera.org:8080/4864
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Henry Robinson <he...@cloudera.com>
---
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
2 files changed, 6 insertions(+), 2 deletions(-)
Approvals:
Henry Robinson: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.
Juan Yu has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 1:
I agree with the fix. but just want to clarify the impact. I checked this piece of code before. for each fragment we guarantee the status will be reported at least once no matter report thread running or not, right?
void PlanFragmentExecutor::FragmentComplete() {
// Check the atomic flag. If it is set, then a fragment complete report has already
// been sent.
bool send_report = completed_report_sent_.CompareAndSwap(0, 1);
...
if (send_report) SendReport(true);
}
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4383: Ensure plan fragment report thread is always started
......................................................................
Patch Set 2: Code-Review+2 Verified+1
Exhaustive build passed, GVO passed in conjunction with https://gerrit.cloudera.org/#/c/4865/
--
To view, visit http://gerrit.cloudera.org:8080/4864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d3cab4cc5245014758e6b70dec7a706a7fa929b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No