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