[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 19:09:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..

IMPALA-7055: fix race with DML errors

Error statuses could be lost because backend_exec_complete_barrier_
went to 0 before the query was transitioned to an error state.
Reordering the UpdateExecState() and backend_exec_complete_barrier_
calls prevents this race.

Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Reviewed-on: http://gerrit.cloudera.org:8080/10491
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins 
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
2 files changed, 19 insertions(+), 8 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3:

It looks like that flake occurred on a different branch too. I'll try to merge 
this again since it fixes a test failure.


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 15:43:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2547/


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 15:43:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3:

Hit IMPALA-7067. Will hold off on merging in case this patch increased the 
changes of hitting that flakiness.


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 04:59:24 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2540/


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 04:20:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2540/


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:52:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:52:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:30:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 1:

(2 comments)

Maybe take another look to make sure that my change was what you intended

http://gerrit.cloudera.org:8080/#/c/10491/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10491/1//COMMIT_MSG@12
PS1, Line 12: Updating backend_exec_complete_barrier_ isn't actually necessary
: when handling error
> BTW, that's kind of true, except that if we've already seen an error then w
Yeah you're right, it is necessary if the state was already RETURNED_RESULTS 
and we got an error after that transition.


http://gerrit.cloudera.org:8080/#/c/10491/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10491/1/be/src/runtime/coordinator.cc@688
PS1, Line 688: if (VLOG_QUERY_IS_ON && pending_backends >= 0) {
 :   VLOG_QUERY << "Backend completed:"
 :  << " host=" << 
TNetworkAddressToString(backend_state->impalad_address())
 :  << " remaining=" << pending_backends
 :  << " query_id=" << PrintId(query_id());
 :   BackendState::LogFirstInProgress(backend_states_);
 : }
> I think I messed this up because I wanted this log to print before the logg
Yeah that makes sense actually, put it back up the top.



--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:28:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Hello Bikramjeet Vig, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10491

to look at the new patch set (#2).

Change subject: IMPALA-7055: fix race with DML errors
..

IMPALA-7055: fix race with DML errors

Error statuses could be lost because backend_exec_complete_barrier_
went to 0 before the query was transitioned to an error state.
Reordering the UpdateExecState() and backend_exec_complete_barrier_
calls prevents this race.

Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
2 files changed, 19 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/10491/2
--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10491/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10491/1//COMMIT_MSG@12
PS1, Line 12: Updating backend_exec_complete_barrier_ isn't actually necessary
: when handling error
BTW, that's kind of true, except that if we've already seen an error then we 
won't transition. So, handling an error here doesn't guarantee the notify with 
happen.



--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:04:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 1: Code-Review+2

(1 comment)

Thanks for taking care of that Tim. It looks right.

http://gerrit.cloudera.org:8080/#/c/10491/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/10491/1/be/src/runtime/coordinator.cc@688
PS1, Line 688: if (VLOG_QUERY_IS_ON && pending_backends >= 0) {
 :   VLOG_QUERY << "Backend completed:"
 :  << " host=" << 
TNetworkAddressToString(backend_state->impalad_address())
 :  << " remaining=" << pending_backends
 :  << " query_id=" << PrintId(query_id());
 :   BackendState::LogFirstInProgress(backend_states_);
 : }
I think I messed this up because I wanted this log to print before the logging 
in UpdateExecState() (so it'd be clearer where the error comes from we are 
updating the exec state). Maybe the order isn't actually helpful, but if you 
think it does then an option is to use 
backend_exec_complete_barrier_->pending() to get pending_backends earlier 
without notifying until later.



--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 24 May 2018 00:02:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10491


Change subject: IMPALA-7055: fix race with DML errors
..

IMPALA-7055: fix race with DML errors

Error statuses could be lost because backend_exec_complete_barrier_
went to 0 before the query was transitioned to an error state.

Updating backend_exec_complete_barrier_ isn't actually necessary
when handling error - UpdateExecState() already signals the barrier
when transitioning to an ERROR/CANCELLED terminal state.

Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
2 files changed, 15 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/10491/1
--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7055: fix race with DML errors

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10491 )

Change subject: IMPALA-7055: fix race with DML errors
..


Patch Set 1:

I'm still in the process of testing this but wanted to get feedback on whether 
it's the correct fix.


--
To view, visit http://gerrit.cloudera.org:8080/10491
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafd0b342e77a065be7cc28fa8c8a9df445622c2
Gerrit-Change-Number: 10491
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 23 May 2018 22:14:00 +
Gerrit-HasComments: No