You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sahil Takiar (Code Review)" <ge...@cloudera.org> on 2020/01/16 01:49:52 UTC
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15046
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
IMPALA-9296: Move AuxErrorInfo to StatefulStatus
This patch moves AuxErrorInfoPB from FragmentInstanceExecStatusPB to
StatefulStatusPB. This is necessary because if the report with the
AuxErrorInfoPB is dropped (e.g. due to backpressure at the Coordinator
or a flaky network), the next report won't contain the AuxErrorInfoPB,
and the error info will be lost. StatefulStatus solves this by detecting
any reports that may not have been received by the Coordinator, and
re-transmitting any StatefulStatuses that were not successfully
delivered.
This change also makes the setting of AuxErrorInfoPB stateful, so that
the error info only shows up in one report and is then dropped from the
RuntimeState.
Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M common/protobuf/control_service.proto
5 files changed, 25 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/15046/1
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 19:43:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h@322
PS1, Line 322: This method clears aux_error_info_
> Hmm, I guess I didn't realize this wasn't already the case before this patc
I don't full understand how the protocol works, but it looks like after a certain point FragmentInstanceExecStatusPB are no longer sent. I believe once they are marked as 'done', they said a final report and then thats it.
So the issue I was seeing in IMPALA-9295 was that the final fragment report include the aux error info, but the overall status report didn't have an overall_status set.
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h@446
PS1, Line 446: bool reported_aux_error_info_;
> Might want to explicitly initialize this to false here for clarity.
Done
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc@316
PS1, Line 316: if (aux_error_info_ == nullptr && !reported_aux_error_info_) {
> I'm not sure that this is the right was to do it, since it means that if a
I agree this is a good idea. I think we should do it in a follow up JIRA though. Filed IMPALA-9301.
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 21:15:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5446/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 23:54:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h@322
PS1, Line 322: This method clears aux_error_info_
> I don't full understand how the protocol works, but it looks like after a c
Alright, well like I said I think this is the right approach anyways.
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc@316
PS1, Line 316: if (aux_error_info_ == nullptr && !reported_aux_error_info_) {
> I agree this is a good idea. I think we should do it in a follow up JIRA th
Sounds good. Probably doesn't need to be done before the main transparent query retry patch
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 23:27:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15046
to look at the new patch set (#2).
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
IMPALA-9296: Move AuxErrorInfo to StatefulStatus
This patch moves AuxErrorInfoPB from FragmentInstanceExecStatusPB to
StatefulStatusPB. This is necessary because if the report with the
AuxErrorInfoPB is dropped (e.g. due to backpressure at the Coordinator
or a flaky network), the next report won't contain the AuxErrorInfoPB,
and the error info will be lost. StatefulStatus solves this by detecting
any reports that may not have been received by the Coordinator, and
re-transmitting any StatefulStatuses that were not successfully
delivered.
This change also makes the setting of AuxErrorInfoPB stateful, so that
the error info only shows up in one report and is then dropped from the
RuntimeState.
Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M common/protobuf/control_service.proto
5 files changed, 25 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/15046/2
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5448/ : 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/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 04:28:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5442/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 12:33:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
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/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
IMPALA-9296: Move AuxErrorInfo to StatefulStatus
This patch moves AuxErrorInfoPB from FragmentInstanceExecStatusPB to
StatefulStatusPB. This is necessary because if the report with the
AuxErrorInfoPB is dropped (e.g. due to backpressure at the Coordinator
or a flaky network), the next report won't contain the AuxErrorInfoPB,
and the error info will be lost. StatefulStatus solves this by detecting
any reports that may not have been received by the Coordinator, and
re-transmitting any StatefulStatuses that were not successfully
delivered.
This change also makes the setting of AuxErrorInfoPB stateful, so that
the error info only shows up in one report and is then dropped from the
RuntimeState.
Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Reviewed-on: http://gerrit.cloudera.org:8080/15046
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M common/protobuf/control_service.proto
5 files changed, 25 insertions(+), 14 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sat, 18 Jan 2020 02:05:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h@322
PS1, Line 322: This method clears aux_error_info_
Hmm, I guess I didn't realize this wasn't already the case before this patch. Now I'm confused as to what the issue was in IMPALA-9295, since before this patch it seems like once a AuxErrorInfoPB has been generated, it will be sent with every report.
I think this patch is still the right thing to do though, otherwise if we send the same AuxErrorInfoPB multiple times the coordinator will blacklist the corresponding executor multiple times even if there was really only one rpc failure.
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.h@446
PS1, Line 446: bool reported_aux_error_info_;
Might want to explicitly initialize this to false here for clarity.
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:
http://gerrit.cloudera.org:8080/#/c/15046/1/be/src/runtime/runtime-state.cc@316
PS1, Line 316: if (aux_error_info_ == nullptr && !reported_aux_error_info_) {
I'm not sure that this is the right was to do it, since it means that if a backend sees multiple rpc failures in a single query only one will ever be reported to the coordinator.
Of course, I've been advocating for being aggressive about blacklisting. Suppose there were two rpc failures, then there are two cases here - either both rpcs were to the same other executor, in which case the fact that there were two failures makes us more confident something is going on with that executor and we might actually want to blacklist the executor twice (which will just extend the amount of time that it stays blacklisted for), or the two rpcs were to different executors, in which case if we only blacklist one of them if we then retry the query it may very well fail again.
And even if we do want to stay more conservative about blacklisting, you've suggested before (and I agree) that its generally preferable to report as much info about errors as we've got, and then centralize the logic for deciding how to act on those errors in the coordinator.
Obviously this is all debatable, though.
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 20:02:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5445/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Jan 2020 23:54:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9296: Move AuxErrorInfo to StatefulStatus
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15046 )
Change subject: IMPALA-9296: Move AuxErrorInfo to StatefulStatus
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5438/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabbb48dd3ab58ba7b76b1ab6979b92d0e25e72e3
Gerrit-Change-Number: 15046
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2020 19:43:04 +0000
Gerrit-HasComments: No