You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/04/26 16:39:37 UTC
[1/2] impala git commit: IMPALA-6927: Remove invalid DCHECK from
coordinator backend state
Repository: impala
Updated Branches:
refs/heads/master c557a5bfb -> 518bcd3e1
IMPALA-6927: Remove invalid DCHECK from coordinator backend state
An invalid DCHECK in Coordinator::BackendState::InstanceStatsToJson()
assumed that the number of fragment instances is equal to the number of
fragments. This is not true when mt_dop > 0, which results in multiple
instances of a fragment on a single node.
The fix is to remove the invalid DCHECK. It served to document the
programmers understanding and the code does not rely on the assumption
being true.
This change adds a test that would fail with the DCHECK in place.
Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Reviewed-on: http://gerrit.cloudera.org:8080/10192
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/a377f285
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/a377f285
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/a377f285
Branch: refs/heads/master
Commit: a377f28543f576577ab748c2c3212ffb93f56767
Parents: c557a5b
Author: Lars Volker <lv...@cloudera.com>
Authored: Wed Apr 25 12:35:48 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Thu Apr 26 04:26:50 2018 +0000
----------------------------------------------------------------------
be/src/runtime/coordinator-backend-state.cc | 1 -
tests/webserver/test_web_pages.py | 14 +++++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/a377f285/be/src/runtime/coordinator-backend-state.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/coordinator-backend-state.cc b/be/src/runtime/coordinator-backend-state.cc
index c701f8e..303a5a3 100644
--- a/be/src/runtime/coordinator-backend-state.cc
+++ b/be/src/runtime/coordinator-backend-state.cc
@@ -616,7 +616,6 @@ void Coordinator::BackendState::InstanceStatsToJson(Value* value, Document* docu
elem.second->ToJson(&val, document);
instance_stats.PushBack(val, document->GetAllocator());
}
- DCHECK_EQ(instance_stats.Size(), fragments_.size());
}
value->AddMember("instance_stats", instance_stats, document->GetAllocator());
http://git-wip-us.apache.org/repos/asf/impala/blob/a377f285/tests/webserver/test_web_pages.py
----------------------------------------------------------------------
diff --git a/tests/webserver/test_web_pages.py b/tests/webserver/test_web_pages.py
index c736cb3..60deca4 100644
--- a/tests/webserver/test_web_pages.py
+++ b/tests/webserver/test_web_pages.py
@@ -181,9 +181,11 @@ class TestWebPage(ImpalaTestSuite):
self.get_and_check_status(self.TABLE_METRICS_URL +
"?name=%s.%s" % (db_name, tbl_name), metric, ports_to_test=self.CATALOG_TEST_PORT)
- def __run_query_and_get_debug_page(self, query, page_url):
+ def __run_query_and_get_debug_page(self, query, page_url, query_options=None):
"""Runs a query to obtain the content of the debug page pointed to by page_url, then
cancels the query."""
+ if query_options:
+ self.client.set_configuration(query_options)
query_handle = self.client.execute_async(query)
response_json = ""
try:
@@ -225,6 +227,16 @@ class TestWebPage(ImpalaTestSuite):
else:
assert 'backend_instances' not in response_json
+ def test_backend_instances_mt_dop(self, unique_database):
+ """Test that accessing /query_finstances does not crash the backend when running with
+ mt_dop."""
+ # vector.get_value('exec_option')['mt_dop'] = 4
+ QUERY = "select * from tpch.lineitem where l_orderkey < 3"
+ QUERY_OPTIONS = dict(mt_dop=4)
+ response_json = self.__run_query_and_get_debug_page(QUERY, self.QUERY_FINSTANCES_URL,
+ QUERY_OPTIONS)
+ assert len(response_json['backend_instances']) > 0
+
def test_io_mgr_threads(self):
"""Test that IoMgr threads have readable names. This test assumed that all systems we
support have a disk called 'sda'."""
[2/2] impala git commit: IMPALA-6892: CheckHashAndDecrypt() includes
file and host
Posted by ta...@apache.org.
IMPALA-6892: CheckHashAndDecrypt() includes file and host
The error text with AES-GCM enabled looks like:
Error reading 44 bytes from scratch file
'/tmp/impala-scratch/0:0_d43635d0-8f55-485e-8899-907af289ac86' on
backend tarmstrong-box:22000 at offset 0: verification of read data
failed.
OpenSSL error in EVP_DecryptFinal:
139634997483216:error:0607C083:digital envelope
routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
139634997483216:error:0607C083:digital envelope
routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
139634997483216:error:0607C083:digital envelope
routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
139634997483216:error:0607C083:digital envelope
routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
Testing:
Added a backend test to exercise the code path and verify the error
code.
Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Reviewed-on: http://gerrit.cloudera.org:8080/10204
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/518bcd3e
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/518bcd3e
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/518bcd3e
Branch: refs/heads/master
Commit: 518bcd3e148caa8b42011de11e971c2978fb6f3b
Parents: a377f28
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Wed Apr 25 11:32:30 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Thu Apr 26 07:14:30 2018 +0000
----------------------------------------------------------------------
be/src/exec/exec-node.cc | 3 --
be/src/runtime/tmp-file-mgr-test.cc | 53 ++++++++++++++++++++++++++++++
be/src/runtime/tmp-file-mgr.cc | 15 +++++++--
common/thrift/generate_error_codes.py | 4 +++
4 files changed, 70 insertions(+), 5 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/518bcd3e/be/src/exec/exec-node.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/exec-node.cc b/be/src/exec/exec-node.cc
index bf08256..d2dd85f 100644
--- a/be/src/exec/exec-node.cc
+++ b/be/src/exec/exec-node.cc
@@ -68,9 +68,6 @@
using strings::Substitute;
-DECLARE_int32(be_port);
-DECLARE_string(hostname);
-
namespace impala {
const string ExecNode::ROW_THROUGHPUT_COUNTER = "RowsReturnedRate";
http://git-wip-us.apache.org/repos/asf/impala/blob/518bcd3e/be/src/runtime/tmp-file-mgr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/tmp-file-mgr-test.cc b/be/src/runtime/tmp-file-mgr-test.cc
index a61f844..318fdbe 100644
--- a/be/src/runtime/tmp-file-mgr-test.cc
+++ b/be/src/runtime/tmp-file-mgr-test.cc
@@ -31,6 +31,7 @@
#include "service/fe-support.h"
#include "testutil/gtest-util.h"
#include "util/condition-variable.h"
+#include "util/cpu-info.h"
#include "util/filesystem-util.h"
#include "util/metrics.h"
@@ -169,6 +170,9 @@ class TmpFileMgrTest : public ::testing::Test {
while (cb_counter_ < val) cb_cv_.Wait(lock);
}
+ /// Implementation of TestBlockVerification(), which is run with different environments.
+ void TestBlockVerification();
+
ObjectPool obj_pool_;
scoped_ptr<MetricGroup> metrics_;
// Owned by 'obj_pool_'.
@@ -507,6 +511,55 @@ TEST_F(TmpFileMgrTest, TestEncryptionDuringCancellation) {
file_group.Close();
test_env_->TearDownQueries();
}
+
+TEST_F(TmpFileMgrTest, TestBlockVerification) {
+ TestBlockVerification();
+}
+
+TEST_F(TmpFileMgrTest, TestBlockVerificationGcmDisabled) {
+ // Disable AES-GCM to test that errors from non-AES-GCM verification are also correct.
+ CpuInfo::TempDisable t1(CpuInfo::PCLMULQDQ);
+ TestBlockVerification();
+}
+
+void TmpFileMgrTest::TestBlockVerification() {
+ FLAGS_disk_spill_encryption = true;
+ TUniqueId id;
+ TmpFileMgr::FileGroup file_group(test_env_->tmp_file_mgr(), io_mgr(), profile_, id);
+ string data = "the quick brown fox jumped over the lazy dog";
+ MemRange data_mem_range(reinterpret_cast<uint8_t*>(&data[0]), data.size());
+
+ // Start a write in flight, which should encrypt the data and write it to disk.
+ unique_ptr<TmpFileMgr::WriteHandle> handle;
+ WriteRange::WriteDoneCallback callback =
+ bind(mem_fn(&TmpFileMgrTest::SignalCallback), this, _1);
+ ASSERT_OK(file_group.Write(data_mem_range, callback, &handle));
+ string file_path = handle->TmpFilePath();
+
+ WaitForWrite(handle.get());
+ WaitForCallbacks(1);
+
+ // Modify the data in the scratch file and check that a read error occurs.
+ FILE* file = fopen(file_path.c_str(), "rb+");
+ fputc('?', file);
+ fclose(file);
+ vector<uint8_t> tmp;
+ tmp.resize(data.size());
+ Status read_status = file_group.Read(handle.get(), MemRange(tmp.data(), tmp.size()));
+ LOG(INFO) << read_status.GetDetail();
+ EXPECT_EQ(TErrorCode::SCRATCH_READ_VERIFY_FAILED, read_status.code())
+ << read_status.GetDetail();
+
+ // Modify the data in memory. Restoring the data should fail.
+ data[0] = '?';
+ Status restore_status = file_group.RestoreData(move(handle), data_mem_range);
+ LOG(INFO) << restore_status.GetDetail();
+ EXPECT_EQ(TErrorCode::SCRATCH_READ_VERIFY_FAILED, restore_status.code())
+ << restore_status.GetDetail();
+
+ file_group.Close();
+ test_env_->TearDownQueries();
+}
}
int main(int argc, char** argv) {
http://git-wip-us.apache.org/repos/asf/impala/blob/518bcd3e/be/src/runtime/tmp-file-mgr.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/tmp-file-mgr.cc b/be/src/runtime/tmp-file-mgr.cc
index 5adfcab..b44a5c4 100644
--- a/be/src/runtime/tmp-file-mgr.cc
+++ b/be/src/runtime/tmp-file-mgr.cc
@@ -608,15 +608,26 @@ Status TmpFileMgr::WriteHandle::EncryptAndHash(MemRange buffer) {
Status TmpFileMgr::WriteHandle::CheckHashAndDecrypt(MemRange buffer) {
DCHECK(FLAGS_disk_spill_encryption);
+ DCHECK(write_range_ != nullptr);
SCOPED_TIMER(encryption_timer_);
// GCM mode will verify the integrity by itself
if (!key_.IsGcmMode()) {
if (!hash_.Verify(buffer.data(), buffer.len())) {
- return Status("Block verification failure");
+ return Status(TErrorCode::SCRATCH_READ_VERIFY_FAILED, buffer.len(),
+ write_range_->file(), GetBackendString(), write_range_->offset());
}
}
- return key_.Decrypt(buffer.data(), buffer.len(), buffer.data());
+ Status decrypt_status = key_.Decrypt(buffer.data(), buffer.len(), buffer.data());
+ if (!decrypt_status.ok()) {
+ // Treat decryption failing as a verification failure, but include extra info from
+ // the decryption status.
+ Status result_status(TErrorCode::SCRATCH_READ_VERIFY_FAILED, buffer.len(),
+ write_range_->file(), GetBackendString(), write_range_->offset());
+ result_status.MergeStatus(decrypt_status);
+ return result_status;
+ }
+ return Status::OK();
}
string TmpFileMgr::WriteHandle::DebugString() {
http://git-wip-us.apache.org/repos/asf/impala/blob/518bcd3e/common/thrift/generate_error_codes.py
----------------------------------------------------------------------
diff --git a/common/thrift/generate_error_codes.py b/common/thrift/generate_error_codes.py
index fc47b2a..7775ef6 100755
--- a/common/thrift/generate_error_codes.py
+++ b/common/thrift/generate_error_codes.py
@@ -359,6 +359,10 @@ error_codes = (
("LIB_VERSION_MISMATCH", 117,
"The library $0 last modified time $1 does not match the expected last "
"modified time $2. Run 'refresh functions <db name>'."),
+
+ ("SCRATCH_READ_VERIFY_FAILED", 118, "Error reading $0 bytes from scratch file '$1' "
+ "on backend $2 at offset $3: verification of read data failed."),
+
)
import sys