Repository: incubator-impala
Updated Branches:
refs/heads/master 9f678a742 -> d7d6c0367
IMPALA-5427: Fix race between CRS::UpdateQueryStatus() and beeswax RPCs
There was a race between ClientRequestState::UpdateQueryStatus() and the
beeswax get_state()/get_log() RPCs leading to the rare situation that a
query would abort with an error, but the error message would be empty.
The fix is to take the ClientRequestState lock in the beeswax RPCs
before obtaining the status.
To test this I ran test_corrupt_files in a loop for a day. Without this
fix, it would usually fail within a few hours.
I changed the test to allow running it in parallel like so:
@pytest.mark.parametrize('multiplier', xrange(32))
def test_corrupt_files(self, vector, multiplier):
Then I ran it in a loop like so:
i=0; while [ $? -eq 0 ]; do ((++i)); echo "Run: $i"; impala-py.test \
tests/query_test/test_scanners.py::TestParquet::test_corrupt_files \
--exploration_strategy=exhaustive -n8; done
Change-Id: Ib4494fe3f933cc23841db0e7da407eec5650f2b5
Reviewed-on: http://gerrit.cloudera.org:8080/7155
Reviewed-by: Lars Volker <[email protected]>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/d7d6c036
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/d7d6c036
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/d7d6c036
Branch: refs/heads/master
Commit: d7d6c036747ed7db8affd3a3fec524fd109ac6db
Parents: 9f678a7
Author: Lars Volker <[email protected]>
Authored: Wed Jun 7 15:31:15 2017 -0700
Committer: Impala Public Jenkins <[email protected]>
Committed: Tue Jul 4 02:05:59 2017 +0000
----------------------------------------------------------------------
be/src/service/impala-beeswax-server.cc | 47 +++++++++++++++++-----------
1 file changed, 28 insertions(+), 19 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/d7d6c036/be/src/service/impala-beeswax-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-beeswax-server.cc
b/be/src/service/impala-beeswax-server.cc
index a867d14..026a06e 100644
--- a/be/src/service/impala-beeswax-server.cc
+++ b/be/src/service/impala-beeswax-server.cc
@@ -17,8 +17,6 @@
#include "service/impala-server.h"
-#include <boost/algorithm/string/join.hpp>
-
#include "common/logging.h"
#include "gen-cpp/Frontend_types.h"
#include "rpc/thrift-util.h"
@@ -36,8 +34,6 @@
#include "common/names.h"
-using boost::adopt_lock_t;
-using boost::algorithm::join;
using namespace apache::thrift;
using namespace apache::hive::service::cli::thrift;
using namespace beeswax;
@@ -120,13 +116,15 @@ void ImpalaServer::executeAndWait(QueryHandle&
query_handle, const Query& query,
}
// block until results are ready
request_state->Wait();
- status = request_state->query_status();
+ {
+ lock_guard<mutex> l(*request_state->lock());
+ status = request_state->query_status();
+ }
if (!status.ok()) {
(void) UnregisterQuery(request_state->query_id(), false, &status);
RaiseBeeswaxException(status.GetDetail(), SQLSTATE_GENERAL_ERROR);
}
- request_state->UpdateNonErrorQueryState(beeswax::QueryState::FINISHED);
TUniqueIdToQueryHandle(request_state->query_id(), &query_handle);
// If the input log context id is an empty string, then create a new number
and
@@ -244,17 +242,18 @@ beeswax::QueryState::type ImpalaServer::get_state(const
QueryHandle& handle) {
QueryHandleToTUniqueId(handle, &query_id);
VLOG_ROW << "get_state(): query_id=" << PrintId(query_id);
- lock_guard<mutex> l(client_request_state_map_lock_);
- ClientRequestStateMap::iterator entry =
client_request_state_map_.find(query_id);
- if (entry != client_request_state_map_.end()) {
- return entry->second->query_state();
- } else {
+ shared_ptr<ClientRequestState> request_state =
GetClientRequestState(query_id);
+ if (UNLIKELY(request_state == nullptr)) {
VLOG_QUERY << "ImpalaServer::get_state invalid handle";
RaiseBeeswaxException(Substitute("Invalid query handle: $0",
PrintId(query_id)),
SQLSTATE_GENERAL_ERROR);
}
- // dummy to keep compiler happy
- return beeswax::QueryState::FINISHED;
+ // Take the lock to ensure that if the client sees a query_state ==
EXCEPTION, it is
+ // guaranteed to see the error query_status.
+ lock_guard<mutex> l(*request_state->lock());
+ DCHECK_EQ(request_state->query_state() == beeswax::QueryState::EXCEPTION,
+ !request_state->query_status().ok());
+ return request_state->query_state();
}
void ImpalaServer::echo(string& echo_string, const string& input_string) {
@@ -285,18 +284,28 @@ void ImpalaServer::get_log(string& log, const
LogContextId& context) {
return;
}
stringstream error_log_ss;
- // If the query status is !ok, include the status error message at the top
of the log.
- if (!request_state->query_status().ok()) {
- error_log_ss << request_state->query_status().GetDetail() << "\n";
+
+ {
+ // Take the lock to ensure that if the client sees a query_state ==
EXCEPTION, it is
+ // guaranteed to see the error query_status.
+ lock_guard<mutex> l(*request_state->lock());
+ DCHECK_EQ(request_state->query_state() == beeswax::QueryState::EXCEPTION,
+ !request_state->query_status().ok());
+ // If the query status is !ok, include the status error message at the top
of the log.
+ if (!request_state->query_status().ok()) {
+ error_log_ss << request_state->query_status().GetDetail() << "\n";
+ }
}
// Add warnings from analysis
- error_log_ss << join(request_state->GetAnalysisWarnings(), "\n");
+ for (const string& warning : request_state->GetAnalysisWarnings()) {
+ error_log_ss << warning << "\n";
+ }
// 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();
+ const std::string coord_errors = request_state->coord()->GetErrorLog();
+ if (!coord_errors.empty()) error_log_ss << coord_errors << "\n";
}
log = error_log_ss.str();
}
