[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Tested-by: Henry Robinson --- 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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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
Gerrit-Reviewer: Juan Yu
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4383: Ensure plan fragment report thread is always started
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 Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[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
