Dan Hecht has posted comments on this change. Change subject: IMPALA-5427: Fix race between CRS::UpdateQueryStatus() and beeswax RPCs ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/7155/3/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS3, Line 291: // Add warnings from analysis : error_log_ss << join(request_state->GetAnalysisWarnings(), "\n"); the write of this doesn't look like it's done under the lock, so no point of reading it under the lock. maybe that should be fixed, but not necessary for this change. PS3, Line 294: // Add warnings from execution : if (request_state->coord() != nullptr) { : if (!request_state->query_status().ok()) error_log_ss << "\n\n"; : error_log_ss << request_state->coord()->GetErrorLog(); : } : log = error_log_ss.str(); I think we shouldn't hold the lock across this since it should be synchronized by the coordinator itself. in other words, until the locking is sorted out better, it's probably only worth holding the lock across reads of request_state->query_status(). that also makes it consistent with HS2 where we don't hold the lock in ImpalaServer::GetLog(). -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
