Lars Volker has posted comments on this change. Change subject: IMPALA-5427: Fix race between CRS::UpdateQueryStatus() and beeswax RPCs ......................................................................
Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/7155/9/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: Line 122: } > It'd be clearer if we just made CRS::Wait() return the status (it can set t Wait() deals with the latest status of WaitInternal, but as far as I could tell that may not be the same as request_state->query_status(). If WaitInternal returns a non-OK status but query_status_ is already non-ok, the latter is not overwritten. Taking the lock again here seems easier to reason about to me. Additionally, making Wait() return a status breaks the symmetry with WaitAsync(). I don't feel strongly about it though, let me know if you'd like me to change it. Line 128: TUniqueIdToQueryHandle(request_state->query_id(), &query_handle); > I think Wait() takes care of this already, no? Yes, removed. Line 252: // guaranteed to see the error query_status. > I think this is subtle enough to warrant a comment like: Done Line 288: { > I think we should comment this too. Done Line 290: // guaranteed to see the error query_status. > I think we should have the query_state == EXCEPTION dcheck here too. Done http://gerrit.cloudera.org:8080/#/c/7155/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1011: ArchiveQuery(*request_state); > why do we need to take the map lock here? It may be a left over from a previous try to add more locks until the error goes away. I removed it and will exercise the tests in a loop again. -- 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: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
