impala-reviews mailing list archives

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

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


Patch Set 1:

> > (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.

Just be careful to not take the CRS::lock_ while still holding the map lock. Follow the same
pattern elsewhere by using GetClientRequestState() and then take the lock_ after owning a
shared ptr to the CLR itself.

-- 
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