impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5427: Fix order of status handling in ClientRequestState
Date Mon, 12 Jun 2017 21:40:04 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5427: Fix order of status handling in ClientRequestState
......................................................................


Patch Set 1: -Code-Review

> (1 comment)
 > 
 > This is really subtle. In order to be correct, you'd need at least
 > a compiler barrier to ensure the writes stay in the intended order.
 > 
 > However, the HS2 version of the RPCs is already taking the request
 > state lock: GetOperationStatus(). I think we should just do the
 > same inside of get_state() and get_log().  Then we can assert, like
 > the HS2 code, that an error state implies a !ok() status.  Is there
 > a reason we don't want to take the lock?

You're right, if the compiler reorders the instructions, this won't be helpful. My prior reasoning
was that get_log() and get_state() could be called multiple times potentially slowing down
the rest of the operations happening under that query if we hold the CRS::lock_. But seeing
as how HS2 takes it, it shouldn't be an issue. 

Also, I didn't notice this earlier, but we take a global lock inside get_log() and get_state()
anyway (client_request_state_map_lock_), so taking a query wide lock shouldn't make these
functions all that much slower.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4494fe3f933cc23841db0e7da407eec5650f2b5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: No

Mime
View raw message