You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/11/22 02:59:35 UTC

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries 

Sailesh Mukil has uploaded a new change for review.

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

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................

IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries

We hold the connection_to_sessions_map_lock_ while closing multiple
sessions, which could map to a large number of queries, which means an
even larger number of fragments. We hold this process wide lock and a
series of other locks while sending cancel RPCs to all the fragments
that fall under the above mentioned category.

This could slow down the responsiveness to the client by the daemon.
Moreover, holding the lock is unnecessary and we can do without it.

This patch fixes this path by dropping the lock before we call
CloseSessionInternal().

Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
---
M be/src/service/impala-server.cc
1 file changed, 18 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................

IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries

We hold the connection_to_sessions_map_lock_ while closing multiple
sessions, which could map to a large number of queries, which means an
even larger number of fragments. We hold this process wide lock and a
series of other locks while sending cancel RPCs to all the fragments
that fall under the above mentioned category.

This could slow down the responsiveness to the client by the daemon.
Moreover, holding the lock is unnecessary and we can do without it.

This patch fixes this path by dropping the lock before we call
CloseSessionInternal().

Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
2 files changed, 27 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 1:

Change looks safe to me. We should update the comment in impala-server.h to reflect that it's now a terminal lock

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries

We hold the connection_to_sessions_map_lock_ while closing multiple
sessions, which could map to a large number of queries, which means an
even larger number of fragments. We hold this process wide lock and a
series of other locks while sending cancel RPCs to all the fragments
that fall under the above mentioned category.

This could slow down the responsiveness to the client by the daemon.
Moreover, holding the lock is unnecessary and we can do without it.

This patch fixes this path by dropping the lock before we call
CloseSessionInternal().

Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Reviewed-on: http://gerrit.cloudera.org:8080/5173
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
2 files changed, 25 insertions(+), 17 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Sailesh Mukil: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#3).

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................

IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries

We hold the connection_to_sessions_map_lock_ while closing multiple
sessions, which could map to a large number of queries, which means an
even larger number of fragments. We hold this process wide lock and a
series of other locks while sending cancel RPCs to all the fragments
that fall under the above mentioned category.

This could slow down the responsiveness to the client by the daemon.
Moreover, holding the lock is unnecessary and we can do without it.

This patch fixes this path by dropping the lock before we call
CloseSessionInternal().

Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
2 files changed, 25 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/5173/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5173/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1671:     }
why not: sessions_to_close = it->second; ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 4: Code-Review+2

Rebase, carry +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 1:

Stress test running successfully for 5 hours so far.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5173/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1671:   }
> why not: sessions_to_close = it->second; ?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5173/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 1669:     sessions_to_close = it->second;
Not a big deal, but we could do a move() here to avoid the copy.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4516: Don't hold process wide lock connection to sessions map lock while cancelling queries

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No