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 race between CRS::UpdateQueryStatus() and beeswax RPCs
Date Tue, 13 Jun 2017 00:24:52 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message