You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2019/10/24 00:54:26 UTC

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread 

Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14539


Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................

IMPALA-8995: Fix synchronization in dequeue thread

The admission controller's dequeue thread currently wakes up either
when queries release their admission resources or when a statestore
update is received. The dequeue loop releases the admission lock at
the end of the loop, then acquires it back and calls wait on it.
In this small window, a query can complete, update the admission
stats by acquiring the admission lock, and then call send a notify
to wake the dequeue thread. But since the dequeue thread has not
called wait yet, it can miss this notify. Moreover if the statestore
is down there is no way of waking it up. This will cause the queued
queries to eventually timeout. This patch attempts to fix this by
removing that window.

Testing:
Was able to trigger this manually by adding a sleep right before
the dequeue loop acquires the admission lock.

Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
---
M be/src/scheduling/admission-controller.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 1:

(1 comment)

Fix makes sense, just want to be sure that we're not leaving things in a state where more subtle bugs will go un-noticed.

http://gerrit.cloudera.org:8080/#/c/14539/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/14539/1/be/src/scheduling/admission-controller.cc@1375
PS1, Line 1375:     dequeue_cv_.Wait(lock);
I feel like this usage pattern of condition variables is really asking for trouble - the pattern of

  while (!condition) {
    cv_.wait(lock);
  }

Is a lot more robust. Can we switch to using this pattern so that it's more explicit, e.g. have a pending_dequeue_ variable that is explicitly set when signalling the variable to make sure that the wakeup doesn't get lost.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 01:10:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

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/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................

IMPALA-8995: Fix synchronization in dequeue thread

The admission controller's dequeue thread currently wakes up either
when queries release their admission resources or when a statestore
update is received. The dequeue loop releases the admission lock at
the end of the loop, then acquires it back and calls wait on it.
In this small window, a query can complete, update the admission
stats by acquiring the admission lock, and then call send a notify
to wake the dequeue thread. But since the dequeue thread has not
called wait yet, it can miss this notify. Moreover if the statestore
is down there is no way of waking it up. This will cause the queued
queries to eventually timeout. This patch attempts to fix this by
removing that window.

Testing:
Was able to trigger this manually by adding a sleep right before
the dequeue loop acquires the admission lock.

Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Reviewed-on: http://gerrit.cloudera.org:8080/14539
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
2 files changed, 13 insertions(+), 2 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 01:29:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 20:58:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14539/1/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:

http://gerrit.cloudera.org:8080/#/c/14539/1/be/src/scheduling/admission-controller.cc@1375
PS1, Line 1375:     dequeue_cv_.Wait(lock);
> I feel like this usage pattern of condition variables is really asking for 
great suggestion, done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 20:21:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5134/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 20:58:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 20:39:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4876/ : 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/14539
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 21:10:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Bikramjeet Vig (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/14539

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

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................

IMPALA-8995: Fix synchronization in dequeue thread

The admission controller's dequeue thread currently wakes up either
when queries release their admission resources or when a statestore
update is received. The dequeue loop releases the admission lock at
the end of the loop, then acquires it back and calls wait on it.
In this small window, a query can complete, update the admission
stats by acquiring the admission lock, and then call send a notify
to wake the dequeue thread. But since the dequeue thread has not
called wait yet, it can miss this notify. Moreover if the statestore
is down there is no way of waking it up. This will cause the queued
queries to eventually timeout. This patch attempts to fix this by
removing that window.

Testing:
Was able to trigger this manually by adding a sleep right before
the dequeue loop acquires the admission lock.

Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
---
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
2 files changed, 13 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8995: Fix synchronization in dequeue thread

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14539 )

Change subject: IMPALA-8995: Fix synchronization in dequeue thread
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4864/ : 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/14539
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91080ce54e59cc7e6361f7c50d6b2156a8a180c8
Gerrit-Change-Number: 14539
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 01:39:48 +0000
Gerrit-HasComments: No