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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: No
