You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2020/04/01 14:38:35 UTC

[impala] branch master updated (4e6780e -> 5acce4d)

This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git.


    from 4e6780e  IMPALA-2563: Support LDAP search bind operations
     new 5c54151  IMPALA-9582: Upgrade thrift_sasl to 0.4.2 for impala-shell
     new 5acce4d  IMPALA-9472,IMPALA-9473: Add per-partition metrics for data cache

The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 LICENSE.txt                                        |   2 +-
 be/src/runtime/io/data-cache.cc                    |  94 +++++++++++++++++--
 be/src/runtime/io/data-cache.h                     |  18 +++-
 be/src/runtime/io/hdfs-file-reader.cc              |   2 +
 be/src/util/impalad-metrics.cc                     |  30 ++++++
 be/src/util/impalad-metrics.h                      |  25 +++++
 bin/rat_exclude_files.txt                          |   2 +-
 common/thrift/metrics.json                         | 102 ++++++++++++++++++++-
 infra/python/deps/compiled-requirements.txt        |   2 +-
 shell/.gitignore                                   |   6 +-
 .../CHANGELOG.md                                   |   5 +
 .../LICENSE                                        |   0
 .../README.md                                      |   0
 .../setup.py                                       |   2 +-
 .../thrift_sasl/__init__.py                        |  17 +++-
 shell/packaging/requirements.txt                   |   2 +-
 tests/custom_cluster/test_data_cache.py            |  22 +++++
 17 files changed, 309 insertions(+), 22 deletions(-)
 rename shell/ext-py/{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/CHANGELOG.md (64%)
 rename shell/ext-py/{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/LICENSE (100%)
 rename shell/ext-py/{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/README.md (100%)
 rename shell/ext-py/{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/setup.py (98%)
 rename shell/ext-py/{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/thrift_sasl/__init__.py (94%)


[impala] 02/02: IMPALA-9472, IMPALA-9473: Add per-partition metrics for data cache

Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 5acce4d2000485b9cd87a545b2a05154c889181c
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Mon Mar 2 10:37:43 2020 -0800

    IMPALA-9472,IMPALA-9473: Add per-partition metrics for data cache
    
    This adds two sets of metrics. The first is per-partition metrics
    to track the performance of the underlying filesystem for the
    data cache. It keeps histograms of read, write, and eviction
    latency for each data cache partition along with another metric
    recording the path for the partition. These are exposed as the
    following metrics:
    impala-server.io-mgr.remote-data-cache-partition-$0.path
    impala-server.io-mgr.remote-data-cache-partition-$0.read-latency
    impala-server.io-mgr.remote-data-cache-partition-$0.write-latency
    impala-server.io-mgr.remote-data-cache-partition-$0.eviction-latency
    
    This also adds metrics to keep counts of hits, misses, and entries
    in the data cache. Since reducing the latency of IO is an important
    feature of the data cache, the absolute count of hits and misses
    is as important as the hit bytes and miss bytes. This adds the
    following metrics:
    impala-server.io-mgr.remote-data-cache-hit-count
    impala-server.io-mgr.remote-data-cache-miss-count
    impala-server.io-mgr.remote-data-cache-num-entries
    
    To track metrics around inserts, this also adds the following
    metrics:
    impala-server.io-mgr.remote-data-cache-num-inserts
    impala-server.io-mgr.remote-data-cache-dropped-entries
    impala-server.io-mgr.remote-data-cache-instant-evictions
    An instant eviction happens when inserting an entry into the cache
    fails and the entry is immediately evicted during insert. This is
    currently only possible for LIRS when the entry's size is larger
    than the unprotected capacity. This manifests when the cache
    size is very small. For example, for an 8MB entry, this would
    manifest when a cache shard is smaller than 160MB. This metric
    is primarily for debugging.
    
    Testing:
     - Hand testing to verify the per-partition latency histograms
     - Modified custom_cluster/test_data_cache.py to also test
       the counts.
    
    Change-Id: I56a57d75ff11f00ebc85b85bcaf104fb8108c478
    Reviewed-on: http://gerrit.cloudera.org:8080/15382
    Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/runtime/io/data-cache.cc         |  94 ++++++++++++++++++++++++++---
 be/src/runtime/io/data-cache.h          |  18 +++++-
 be/src/runtime/io/hdfs-file-reader.cc   |   2 +
 be/src/util/impalad-metrics.cc          |  30 ++++++++++
 be/src/util/impalad-metrics.h           |  25 ++++++++
 common/thrift/metrics.json              | 102 +++++++++++++++++++++++++++++++-
 tests/custom_cluster/test_data_cache.py |  22 +++++++
 7 files changed, 284 insertions(+), 9 deletions(-)

diff --git a/be/src/runtime/io/data-cache.cc b/be/src/runtime/io/data-cache.cc
index 71edb78..febb6d9 100644
--- a/be/src/runtime/io/data-cache.cc
+++ b/be/src/runtime/io/data-cache.cc
@@ -42,11 +42,13 @@
 #include "util/error-util.h"
 #include "util/filesystem-util.h"
 #include "util/hash-util.h"
+#include "util/histogram-metric.h"
 #include "util/impalad-metrics.h"
 #include "util/metrics.h"
 #include "util/parse-util.h"
 #include "util/pretty-printer.h"
 #include "util/scope-exit-trigger.h"
+#include "util/test-info.h"
 #include "util/uid-util.h"
 
 #ifndef FALLOC_FL_PUNCH_HOLE
@@ -101,6 +103,14 @@ static const int64_t PAGE_SIZE = 1L << 12;
 const char* DataCache::Partition::CACHE_FILE_PREFIX = "impala-cache-file-";
 const char* DataCache::Partition::TRACE_FILE_NAME = "impala-cache-trace.txt";
 const int MAX_FILE_DELETER_QUEUE_SIZE = 500;
+static const char* PARTITION_PATH_METRIC_KEY_TEMPLATE =
+    "impala-server.io-mgr.remote-data-cache-partition-$0.path";
+static const char* PARTITION_READ_LATENCY_METRIC_KEY_TEMPLATE =
+    "impala-server.io-mgr.remote-data-cache-partition-$0.read-latency";
+static const char* PARTITION_WRITE_LATENCY_METRIC_KEY_TEMPLATE =
+    "impala-server.io-mgr.remote-data-cache-partition-$0.write-latency";
+static const char* PARTITION_EVICTION_LATENCY_METRIC_KEY_TEMPLATE =
+    "impala-server.io-mgr.remote-data-cache-partition-$0.eviction-latency";
 
 
 namespace {
@@ -452,8 +462,9 @@ static Cache::EvictionPolicy GetCacheEvictionPolicy(const std::string& policy_st
 }
 
 DataCache::Partition::Partition(
-    const string& path, int64_t capacity, int max_opened_files)
-  : path_(path),
+    int32_t index, const string& path, int64_t capacity, int max_opened_files)
+  : index_(index),
+    path_(path),
     capacity_(max<int64_t>(capacity, PAGE_SIZE)),
     max_opened_files_(max_opened_files),
     meta_cache_(NewCache(GetCacheEvictionPolicy(FLAGS_data_cache_eviction_policy),
@@ -522,12 +533,60 @@ Status DataCache::Partition::Init() {
     RETURN_IF_ERROR(tracer_->Start());
   }
 
+  // Create metrics for this partition
+  InitMetrics();
+
   // Create a backing file for the partition.
   RETURN_IF_ERROR(CreateCacheFile());
   oldest_opened_file_ = 0;
   return Status::OK();
 }
 
+void DataCache::Partition::InitMetrics() {
+  const string& i_string = Substitute("$0", index_);
+  // Backend tests may instantiate the data cache (and its associated partitions)
+  // more than once. If the metrics already exist, then we just need to look up the
+  // metrics to populate the partition's fields.
+  if (TestInfo::is_test() &&
+      ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<StringProperty>(
+          Substitute(PARTITION_PATH_METRIC_KEY_TEMPLATE, i_string)) != nullptr) {
+    // If the partition path metric already initialized, then all the other metrics
+    // must be initialized.
+    read_latency_ =
+      ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
+          Substitute(PARTITION_READ_LATENCY_METRIC_KEY_TEMPLATE, i_string));
+    write_latency_ =
+      ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
+          Substitute(PARTITION_WRITE_LATENCY_METRIC_KEY_TEMPLATE, i_string));
+    eviction_latency_ =
+      ImpaladMetrics::IO_MGR_METRICS->FindMetricForTesting<HistogramMetric>(
+          Substitute(PARTITION_EVICTION_LATENCY_METRIC_KEY_TEMPLATE, i_string));
+    DCHECK(read_latency_ != nullptr);
+    DCHECK(write_latency_ != nullptr);
+    DCHECK(eviction_latency_ != nullptr);
+    return;
+  }
+  // Two cases:
+  // - This is not a backend test, so metrics should only be initialized once.
+  // - This is a backend test, but none of the metrics have been initialized before.
+  DCHECK(read_latency_ == nullptr);
+  DCHECK(write_latency_ == nullptr);
+  DCHECK(eviction_latency_ == nullptr);
+  int64_t ONE_HOUR_IN_NS = 60L * 60L * NANOS_PER_SEC;
+  ImpaladMetrics::IO_MGR_METRICS->AddProperty<string>(
+      PARTITION_PATH_METRIC_KEY_TEMPLATE, path_, i_string);
+  read_latency_ = ImpaladMetrics::IO_MGR_METRICS->RegisterMetric(new HistogramMetric(
+      MetricDefs::Get(PARTITION_READ_LATENCY_METRIC_KEY_TEMPLATE, i_string),
+      ONE_HOUR_IN_NS, 3));
+  write_latency_ = ImpaladMetrics::IO_MGR_METRICS->RegisterMetric(new HistogramMetric(
+      MetricDefs::Get(PARTITION_WRITE_LATENCY_METRIC_KEY_TEMPLATE, i_string),
+      ONE_HOUR_IN_NS, 3));
+  eviction_latency_ =
+      ImpaladMetrics::IO_MGR_METRICS->RegisterMetric(new HistogramMetric(
+          MetricDefs::Get(PARTITION_EVICTION_LATENCY_METRIC_KEY_TEMPLATE, i_string),
+          ONE_HOUR_IN_NS, 3));
+}
+
 Status DataCache::Partition::CloseFilesAndVerifySizes() {
   int64_t total_size = 0;
   for (auto& file : cache_files_) {
@@ -585,7 +644,12 @@ int64_t DataCache::Partition::Lookup(const CacheKey& cache_key, int64_t bytes_to
   bytes_to_read = min(entry.len(), bytes_to_read);
   VLOG(3) << Substitute("Reading file $0 offset $1 len $2 checksum $3 bytes_to_read $4",
       cache_file->path(), entry.offset(), entry.len(), entry.checksum(), bytes_to_read);
-  if (UNLIKELY(!cache_file->Read(entry.offset(), buffer, bytes_to_read))) {
+  bool read_success;
+  {
+    ScopedHistogramTimer read_timer(read_latency_);
+    read_success = cache_file->Read(entry.offset(), buffer, bytes_to_read);
+  }
+  if (UNLIKELY(!read_success)) {
     meta_cache_->Erase(key);
     return 0;
   }
@@ -633,7 +697,12 @@ bool DataCache::Partition::InsertIntoCache(const Slice& key, CacheFile* cache_fi
   // Write to backing file.
   VLOG(3) << Substitute("Storing file $0 offset $1 len $2 checksum $3 ",
       cache_file->path(), insertion_offset, buffer_len, checksum);
-  if (UNLIKELY(!cache_file->Write(insertion_offset, buffer, buffer_len))) {
+  bool write_success;
+  {
+    ScopedHistogramTimer write_timer(write_latency_);
+    write_success = cache_file->Write(insertion_offset, buffer, buffer_len);
+  }
+  if (UNLIKELY(!write_success)) {
     return false;
   }
 
@@ -641,9 +710,14 @@ bool DataCache::Partition::InsertIntoCache(const Slice& key, CacheFile* cache_fi
   CacheEntry entry(cache_file, insertion_offset, buffer_len, checksum);
   memcpy(meta_cache_->MutableValue(&pending_handle), &entry, sizeof(CacheEntry));
   Cache::UniqueHandle handle(meta_cache_->Insert(std::move(pending_handle), this));
-  // Check for failure of Insert
-  if (UNLIKELY(handle.get() == nullptr)) return false;
+  // Check for failure of Insert(), which means the entry was evicted during Insert()
+  if (UNLIKELY(handle.get() == nullptr)){
+    ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS->Increment(1);
+    return false;
+  }
   ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES->Increment(charge_len);
+  ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES->Increment(1);
+  ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES->Increment(1);
   return true;
 }
 
@@ -679,6 +753,7 @@ bool DataCache::Partition::Store(const CacheKey& cache_key, const uint8_t* buffe
     if (exceed_concurrency ||
         pending_insert_set_.find(key.ToString()) != pending_insert_set_.end()) {
       ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES->Increment(buffer_len);
+      ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES->Increment(1);
       if (tracer_ != nullptr) {
         tracer_->Trace(Tracer::STORE_FAILED_BUSY, cache_key, /*lookup_len=*/-1,
             buffer_len);
@@ -730,12 +805,14 @@ void DataCache::Partition::DeleteOldFiles() {
 
 void DataCache::Partition::EvictedEntry(Slice key, Slice value) {
   if (closed_) return;
+  ScopedHistogramTimer eviction_timer(eviction_latency_);
   // Unpack the cache entry.
   CacheEntry entry(value);
   int64_t eviction_len = BitUtil::RoundUp(entry.len(), PAGE_SIZE);
   DCHECK_EQ(entry.offset() % PAGE_SIZE, 0);
   entry.file()->PunchHole(entry.offset(), eviction_len);
   ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES->Increment(-eviction_len);
+  ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES->Increment(-1);
 }
 
 // TODO: Switch to using CRC32 once we fix the TODO in hash-util.h
@@ -796,13 +873,16 @@ Status DataCache::Init() {
     return Status(Substitute("Misconfigured --data_cache_max_opened_files: $0. Must be "
         "at least $1.", FLAGS_data_cache_max_opened_files, cache_dirs.size()));
   }
+  int32_t partition_idx = 0;
   for (const string& dir_path : cache_dirs) {
     LOG(INFO) << "Adding partition " << dir_path << " with capacity "
               << PrettyPrinter::PrintBytes(capacity);
     std::unique_ptr<Partition> partition =
-        make_unique<Partition>(dir_path, capacity, max_opened_files_per_partition);
+        make_unique<Partition>(partition_idx, dir_path, capacity,
+            max_opened_files_per_partition);
     RETURN_IF_ERROR(partition->Init());
     partitions_.emplace_back(move(partition));
+    ++partition_idx;
   }
   CHECK_GT(partitions_.size(), 0);
 
diff --git a/be/src/runtime/io/data-cache.h b/be/src/runtime/io/data-cache.h
index beb83eb..98c27d4 100644
--- a/be/src/runtime/io/data-cache.h
+++ b/be/src/runtime/io/data-cache.h
@@ -26,6 +26,7 @@
 
 #include "common/status.h"
 #include "util/cache/cache.h"
+#include "util/metrics-fwd.h"
 #include "util/spinlock.h"
 #include "util/thread-pool.h"
 #include "kudu/util/faststring.h"
@@ -193,7 +194,8 @@ class DataCache {
    public:
     /// Creates a partition at the given directory 'path' with quota 'capacity' in bytes.
     /// 'max_opened_files' is the maximum number of opened files allowed per partition.
-    Partition(const std::string& path, int64_t capacity, int max_opened_files);
+    Partition(int32_t index, const std::string& path, int64_t capacity,
+        int max_opened_files);
 
     ~Partition();
 
@@ -251,6 +253,10 @@ class DataCache {
     FRIEND_TEST(DataCacheTest, TestAccessTrace);
     class Tracer;
 
+    /// Index of this partition. This is used for naming metrics or other items that
+    /// need separate values for each partition. It does not impact cache behavior.
+    int32_t index_;
+
     /// The directory path which this partition stores cached data in.
     const std::string path_;
 
@@ -300,6 +306,16 @@ class DataCache {
 
     std::unique_ptr<Tracer> tracer_;
 
+    /// Metrics to track performance of the underlying filesystem for the data cache
+    /// These are all latency histograms for the operations on the data cache files for
+    /// this partition.
+    HistogramMetric* read_latency_ = nullptr;
+    HistogramMetric* write_latency_ = nullptr;
+    HistogramMetric* eviction_latency_ = nullptr;
+
+    /// Initialize the metrics
+    void InitMetrics();
+
     /// Utility function for creating a new backing file in 'path_'. The cache
     /// partition's lock needs to be held when calling this function. Returns
     /// error on failure.
diff --git a/be/src/runtime/io/hdfs-file-reader.cc b/be/src/runtime/io/hdfs-file-reader.cc
index edb655f..2f985cf 100644
--- a/be/src/runtime/io/hdfs-file-reader.cc
+++ b/be/src/runtime/io/hdfs-file-reader.cc
@@ -286,6 +286,7 @@ int64_t HdfsFileReader::ReadDataCache(DataCache* remote_data_cache, int64_t file
       scan_range_->reader_->data_cache_partial_hit_counter_->Add(1);
     }
     ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES->Increment(cached_read);
+    ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT->Increment(1);
   }
   return cached_read;
 }
@@ -301,6 +302,7 @@ void HdfsFileReader::WriteDataCache(DataCache* remote_data_cache, int64_t file_o
   scan_range_->reader_->data_cache_miss_bytes_counter_->Add(bytes_missed);
   scan_range_->reader_->data_cache_miss_counter_->Add(1);
   ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES->Increment(bytes_missed);
+  ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT->Increment(1);
 }
 
 void HdfsFileReader::Close() {
diff --git a/be/src/util/impalad-metrics.cc b/be/src/util/impalad-metrics.cc
index 7238284..e57062f 100644
--- a/be/src/util/impalad-metrics.cc
+++ b/be/src/util/impalad-metrics.cc
@@ -61,12 +61,24 @@ const char* ImpaladMetricKeys::IO_MGR_CACHED_BYTES_READ =
     "impala-server.io-mgr.cached-bytes-read";
 const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES =
     "impala-server.io-mgr.remote-data-cache-hit-bytes";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT =
+    "impala-server.io-mgr.remote-data-cache-hit-count";
 const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES =
     "impala-server.io-mgr.remote-data-cache-miss-bytes";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT =
+    "impala-server.io-mgr.remote-data-cache-miss-count";
 const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES =
     "impala-server.io-mgr.remote-data-cache-total-bytes";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES =
+    "impala-server.io-mgr.remote-data-cache-num-entries";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES =
+    "impala-server.io-mgr.remote-data-cache-num-writes";
 const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES =
     "impala-server.io-mgr.remote-data-cache-dropped-bytes";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES =
+    "impala-server.io-mgr.remote-data-cache-dropped-entries";
+const char* ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS =
+    "impala-server.io-mgr.remote-data-cache-instant-evictions";
 const char* ImpaladMetricKeys::IO_MGR_BYTES_WRITTEN =
     "impala-server.io-mgr.bytes-written";
 const char* ImpaladMetricKeys::IO_MGR_NUM_CACHED_FILE_HANDLES =
@@ -156,8 +168,13 @@ IntCounter* ImpaladMetrics::IO_MGR_LOCAL_BYTES_READ = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_SHORT_CIRCUIT_BYTES_READ = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_CACHED_BYTES_READ = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES = nullptr;
+IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES = nullptr;
+IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT = nullptr;
+IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES = nullptr;
+IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES = nullptr;
+IntCounter* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_BYTES_WRITTEN = nullptr;
 IntCounter* ImpaladMetrics::IO_MGR_CACHED_FILE_HANDLES_REOPENED = nullptr;
 IntCounter* ImpaladMetrics::HEDGED_READ_OPS = nullptr;
@@ -189,6 +206,7 @@ IntGauge* ImpaladMetrics::IO_MGR_NUM_FILE_HANDLES_OUTSTANDING = nullptr;
 IntGauge* ImpaladMetrics::IO_MGR_CACHED_FILE_HANDLES_HIT_COUNT = nullptr;
 IntGauge* ImpaladMetrics::IO_MGR_CACHED_FILE_HANDLES_MISS_COUNT = nullptr;
 IntGauge* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES = nullptr;
+IntGauge* ImpaladMetrics::IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES = nullptr;
 IntGauge* ImpaladMetrics::NUM_FILES_OPEN_FOR_INSERT = nullptr;
 IntGauge* ImpaladMetrics::NUM_QUERIES_REGISTERED = nullptr;
 IntGauge* ImpaladMetrics::RESULTSET_CACHE_TOTAL_NUM_ROWS = nullptr;
@@ -330,12 +348,24 @@ void ImpaladMetrics::CreateMetrics(MetricGroup* m) {
 
   IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES = IO_MGR_METRICS->AddCounter(
       ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT = IO_MGR_METRICS->AddCounter(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT, 0);
   IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES = IO_MGR_METRICS->AddCounter(
       ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT = IO_MGR_METRICS->AddCounter(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT, 0);
   IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES = IO_MGR_METRICS->AddGauge(
       ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES = IO_MGR_METRICS->AddGauge(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES = IO_MGR_METRICS->AddCounter(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES, 0);
   IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES = IO_MGR_METRICS->AddCounter(
       ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES = IO_MGR_METRICS->AddCounter(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES, 0);
+  IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS = IO_MGR_METRICS->AddCounter(
+      ImpaladMetricKeys::IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS, 0);
 
   IO_MGR_CACHED_FILE_HANDLES_HIT_RATIO =
       StatsMetric<uint64_t, StatsType::MEAN>::CreateAndRegister(IO_MGR_METRICS,
diff --git a/be/src/util/impalad-metrics.h b/be/src/util/impalad-metrics.h
index 7f20ccc..a3811f7 100644
--- a/be/src/util/impalad-metrics.h
+++ b/be/src/util/impalad-metrics.h
@@ -79,16 +79,35 @@ class ImpaladMetricKeys {
   /// Total number of bytes read from the remote data cache.
   static const char* IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES;
 
+  /// Total number of cache hits for the remote data cache.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT;
+
   /// Total number of bytes missing from the remote data cache.
   static const char* IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES;
 
+  /// Total number of cache misses for the remote data cache.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT;
+
   /// Current byte size of the remote data cache.
   static const char* IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES;
 
+  /// Current number of entries in the remote data cache.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES;
+
+  /// Total number of writes for the remote data cache.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES;
+
   /// Total number of bytes not inserted into the remote data cache due to
   /// concurrency limit.
   static const char* IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES;
 
+  /// Total number of entries not inserted into the remote data cache due to
+  /// concurrency limit.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES;
+
+  /// Total number of entries evicted immediately from the remote data cache.
+  static const char* IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS;
+
   /// Total number of bytes written to disk by the io mgr (for spilling)
   static const char* IO_MGR_BYTES_WRITTEN;
 
@@ -240,8 +259,13 @@ class ImpaladMetrics {
   static IntCounter* IO_MGR_LOCAL_BYTES_READ;
   static IntCounter* IO_MGR_CACHED_BYTES_READ;
   static IntCounter* IO_MGR_REMOTE_DATA_CACHE_HIT_BYTES;
+  static IntCounter* IO_MGR_REMOTE_DATA_CACHE_HIT_COUNT;
   static IntCounter* IO_MGR_REMOTE_DATA_CACHE_MISS_BYTES;
+  static IntCounter* IO_MGR_REMOTE_DATA_CACHE_MISS_COUNT;
+  static IntCounter* IO_MGR_REMOTE_DATA_CACHE_NUM_WRITES;
   static IntCounter* IO_MGR_REMOTE_DATA_CACHE_DROPPED_BYTES;
+  static IntCounter* IO_MGR_REMOTE_DATA_CACHE_DROPPED_ENTRIES;
+  static IntCounter* IO_MGR_REMOTE_DATA_CACHE_INSTANT_EVICTIONS;
   static IntCounter* IO_MGR_SHORT_CIRCUIT_BYTES_READ;
   static IntCounter* IO_MGR_BYTES_WRITTEN;
   static IntCounter* IO_MGR_CACHED_FILE_HANDLES_REOPENED;
@@ -278,6 +302,7 @@ class ImpaladMetrics {
   static IntGauge* IO_MGR_CACHED_FILE_HANDLES_HIT_COUNT;
   static IntGauge* IO_MGR_CACHED_FILE_HANDLES_MISS_COUNT;
   static IntGauge* IO_MGR_REMOTE_DATA_CACHE_TOTAL_BYTES;
+  static IntGauge* IO_MGR_REMOTE_DATA_CACHE_NUM_ENTRIES;
   static IntGauge* NUM_FILES_OPEN_FOR_INSERT;
   static IntGauge* NUM_QUERIES_REGISTERED;
   static IntGauge* RESULTSET_CACHE_TOTAL_NUM_ROWS;
diff --git a/common/thrift/metrics.json b/common/thrift/metrics.json
index de1a8e1..99e2a78 100644
--- a/common/thrift/metrics.json
+++ b/common/thrift/metrics.json
@@ -490,6 +490,16 @@
     "key": "impala-server.io-mgr.remote-data-cache-hit-bytes"
   },
   {
+    "description": "Total number of hits in the remote data cache.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Hit Count",
+    "units": "UNIT",
+    "kind": "COUNTER",
+    "key": "impala-server.io-mgr.remote-data-cache-hit-count"
+  },
+  {
     "description": "Total number of bytes of misses in the remote data cache.",
     "contexts": [
       "IMPALAD"
@@ -500,6 +510,16 @@
     "key": "impala-server.io-mgr.remote-data-cache-miss-bytes"
   },
   {
+    "description": "Total number of misses in the remote data cache.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Miss Count",
+    "units": "UNIT",
+    "kind": "COUNTER",
+    "key": "impala-server.io-mgr.remote-data-cache-miss-count"
+  },
+  {
     "description": "Current byte size of the remote data cache.",
     "contexts": [
       "IMPALAD"
@@ -510,16 +530,96 @@
     "key": "impala-server.io-mgr.remote-data-cache-total-bytes"
   },
   {
+    "description": "Current number of entries in the remote data cache.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Num Entries",
+    "units": "UNIT",
+    "kind": "GAUGE",
+    "key": "impala-server.io-mgr.remote-data-cache-num-entries"
+  },
+  {
+    "description": "Total number of writes into the remote data cache.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Num Writes",
+    "units": "UNIT",
+    "kind": "COUNTER",
+    "key": "impala-server.io-mgr.remote-data-cache-num-writes"
+  },
+  {
     "description": "Total number of bytes not inserted in remote data cache due to concurrency limit.",
     "contexts": [
       "IMPALAD"
     ],
     "label": "Impala Server Io Mgr Remote Data Cache Bytes Not Inserted Due To Concurrency limit",
-    "units": "UNIT",
+    "units": "BYTES",
     "kind": "COUNTER",
     "key": "impala-server.io-mgr.remote-data-cache-dropped-bytes"
   },
   {
+    "description": "Total number of entries not inserted in remote data cache due to concurrency limit.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Entries Not Inserted Due To Concurrency Limit",
+    "units": "UNIT",
+    "kind": "COUNTER",
+    "key": "impala-server.io-mgr.remote-data-cache-dropped-entries"
+  },
+  {
+    "description": "Total number of instantaneous evictions from the remote data cache. An instantaneous eviction happens when the eviction policy rejects an entry during insert.",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Num Instant Evictions",
+    "units": "UNIT",
+    "kind": "COUNTER",
+    "key": "impala-server.io-mgr.remote-data-cache-instant-evictions"
+  },
+  {
+    "description": "Data Cache Partition Path",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Partition Path",
+    "units": "NONE",
+    "kind": "PROPERTY",
+    "key": "impala-server.io-mgr.remote-data-cache-partition-$0.path"
+  },
+  {
+    "description": "Histogram of read operation times for data cache partition",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Partition Read Latency",
+    "units": "TIME_NS",
+    "kind": "HISTOGRAM",
+    "key": "impala-server.io-mgr.remote-data-cache-partition-$0.read-latency"
+  },
+  {
+    "description": "Histogram of write operation times for data cache partition",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Partition Write Latency",
+    "units": "TIME_NS",
+    "kind": "HISTOGRAM",
+    "key": "impala-server.io-mgr.remote-data-cache-partition-$0.write-latency"
+  },
+  {
+    "description": "Histogram of eviction operation times for data cache partition",
+    "contexts": [
+      "IMPALAD"
+    ],
+    "label": "Impala Server Io Mgr Remote Data Cache Partition Eviction Latency",
+    "units": "TIME_NS",
+    "kind": "HISTOGRAM",
+    "key": "impala-server.io-mgr.remote-data-cache-partition-$0.eviction-latency"
+  },
+  {
     "description": "The number of allocated IO buffers. IO buffers are shared by all queries.",
     "contexts": [
       "IMPALAD"
diff --git a/tests/custom_cluster/test_data_cache.py b/tests/custom_cluster/test_data_cache.py
index e6b5a5f..c14de17 100644
--- a/tests/custom_cluster/test_data_cache.py
+++ b/tests/custom_cluster/test_data_cache.py
@@ -60,9 +60,16 @@ class TestDataCache(CustomClusterTestSuite):
     """
     self.run_test_case('QueryTest/data-cache', vector, unique_database)
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-dropped-bytes') >= 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-dropped-entries') >= 0
+    assert \
+        self.get_metric('impala-server.io-mgr.remote-data-cache-instant-evictions') >= 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-count') > 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-count') > 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-total-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-entries') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-writes') > 0
 
   @pytest.mark.execute_serially
   @CustomClusterTestSuite.with_args(
@@ -87,12 +94,17 @@ class TestDataCache(CustomClusterTestSuite):
     # Do a first run to warm up the cache. Expect no hits.
     self.execute_query(QUERY)
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-bytes') == 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-count') == 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-count') > 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-total-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-entries') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-writes') > 0
 
     # Do a second run. Expect some hits.
     self.execute_query(QUERY)
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-bytes') > 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-count') > 0
 
   @pytest.mark.execute_serially
   @CustomClusterTestSuite.with_args(
@@ -113,8 +125,12 @@ class TestDataCache(CustomClusterTestSuite):
   def __test_data_cache_disablement(self, vector):
     # Verifies that the cache metrics are all zero.
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-bytes') == 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-hit-count') == 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-bytes') == 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-miss-count') == 0
     assert self.get_metric('impala-server.io-mgr.remote-data-cache-total-bytes') == 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-entries') == 0
+    assert self.get_metric('impala-server.io-mgr.remote-data-cache-num-writes') == 0
 
     # Runs a query with the cache disabled and then enabled against multiple file formats.
     # Verifies that the metrics stay at zero when the cache is disabled.
@@ -126,7 +142,13 @@ class TestDataCache(CustomClusterTestSuite):
         assert disable_cache ==\
             (self.get_metric('impala-server.io-mgr.remote-data-cache-miss-bytes') == 0)
         assert disable_cache ==\
+            (self.get_metric('impala-server.io-mgr.remote-data-cache-miss-count') == 0)
+        assert disable_cache ==\
             (self.get_metric('impala-server.io-mgr.remote-data-cache-total-bytes') == 0)
+        assert disable_cache ==\
+            (self.get_metric('impala-server.io-mgr.remote-data-cache-num-entries') == 0)
+        assert disable_cache ==\
+            (self.get_metric('impala-server.io-mgr.remote-data-cache-num-writes') == 0)
 
   @pytest.mark.execute_serially
   @CustomClusterTestSuite.with_args(


[impala] 01/02: IMPALA-9582: Upgrade thrift_sasl to 0.4.2 for impala-shell

Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 5c541512f080327686fe0ad6fcf4ef90ca048007
Author: David Knupp <dk...@cloudera.com>
AuthorDate: Mon Mar 30 18:29:44 2020 -0700

    IMPALA-9582: Upgrade thrift_sasl to 0.4.2 for impala-shell
    
    Change-Id: Iff739ebeaf5b022a7418883b638b5c5d17885f3b
    Reviewed-on: http://gerrit.cloudera.org:8080/15610
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 LICENSE.txt                                             |  2 +-
 bin/rat_exclude_files.txt                               |  2 +-
 infra/python/deps/compiled-requirements.txt             |  2 +-
 shell/.gitignore                                        |  6 +++---
 .../CHANGELOG.md                                        |  5 +++++
 .../{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/LICENSE    |  0
 .../{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/README.md  |  0
 .../{thrift_sasl-0.4.1 => thrift_sasl-0.4.2}/setup.py   |  2 +-
 .../thrift_sasl/__init__.py                             | 17 ++++++++++++-----
 shell/packaging/requirements.txt                        |  2 +-
 10 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/LICENSE.txt b/LICENSE.txt
index 6786ce0..c12850c 100644
--- a/LICENSE.txt
+++ b/LICENSE.txt
@@ -697,7 +697,7 @@ be/src/thirdparty/mustache: Apache 2.0 license
 be/src/thirdparty/pcg-cpp-0.98: Apache 2.0 license
 be/src/expr/hll-bias.h: Apache 2.0 license
 shell/ext-py/sasl-0.1.1: Apache 2.0 license
-shell/ext-py/thrift_sasl-0.4.1: Apache 2.0 license
+shell/ext-py/thrift_sasl-0.4.2: Apache 2.0 license
 
 --------------------------------------------------------------------------------
 
diff --git a/bin/rat_exclude_files.txt b/bin/rat_exclude_files.txt
index 32ec350..8083ad3 100644
--- a/bin/rat_exclude_files.txt
+++ b/bin/rat_exclude_files.txt
@@ -46,7 +46,7 @@ shell/ext-py/prettytable-0.7.1/*
 shell/ext-py/sasl-0.1.1/*
 shell/ext-py/six-1.14.0/*
 shell/ext-py/sqlparse-0.1.19/*
-shell/ext-py/thrift_sasl-0.4.1/*
+shell/ext-py/thrift_sasl-0.4.2/*
 www/d3.v3.min.js
 www/jquery/jquery-3.4.1.min.js
 tests/comparison/leopard/static/css/hljs.css
diff --git a/infra/python/deps/compiled-requirements.txt b/infra/python/deps/compiled-requirements.txt
index 0ff2586..061e286 100644
--- a/infra/python/deps/compiled-requirements.txt
+++ b/infra/python/deps/compiled-requirements.txt
@@ -23,7 +23,7 @@ impyla == 0.16.2
   bitarray == 1.2.1
   sasl == 0.2.1
   six == 1.14.0
-  thrift_sasl == 0.4.1
+  thrift_sasl == 0.4.2
 psutil == 5.6.3
 # Required for Kudu:
   Cython == 0.23.4
diff --git a/shell/.gitignore b/shell/.gitignore
index 60a2275..380b3fd 100644
--- a/shell/.gitignore
+++ b/shell/.gitignore
@@ -13,9 +13,9 @@ ext-py/sqlparse-0.1.19/build/
 ext-py/sqlparse-0.1.19/sqlparse.egg-info/
 ext-py/bitarray-0.9.0/bitarray.egg-info/
 ext-py/bitarray-0.9.0/dist/
-ext-py/thrift_sasl-0.4.1/dist/
-ext-py/thrift_sasl-0.4.1/build/
-ext-py/thrift_sasl-0.4.1/six.egg-info/
+ext-py/thrift_sasl-0.4.2/dist/
+ext-py/thrift_sasl-0.4.2/build/
+ext-py/thrift_sasl-0.4.2/six.egg-info/
 
 # This file is used by buildall.sh to find files that need to be removed during the
 # clean phase. Previous version of deps should be kept here for cleaning otherwise they
diff --git a/shell/ext-py/thrift_sasl-0.4.1/CHANGELOG.md b/shell/ext-py/thrift_sasl-0.4.2/CHANGELOG.md
similarity index 64%
rename from shell/ext-py/thrift_sasl-0.4.1/CHANGELOG.md
rename to shell/ext-py/thrift_sasl-0.4.2/CHANGELOG.md
index 08ac33a..28b90cf 100644
--- a/shell/ext-py/thrift_sasl-0.4.1/CHANGELOG.md
+++ b/shell/ext-py/thrift_sasl-0.4.2/CHANGELOG.md
@@ -1,6 +1,11 @@
 Changelog
 =========
 
+0.4.2
+------
+* **Bug Fixes**
+  - Fixes a bug where Thrift transport was not reading all data (#22)
+
 0.4.1
 ------
 * **Bug Fixes**
diff --git a/shell/ext-py/thrift_sasl-0.4.1/LICENSE b/shell/ext-py/thrift_sasl-0.4.2/LICENSE
similarity index 100%
rename from shell/ext-py/thrift_sasl-0.4.1/LICENSE
rename to shell/ext-py/thrift_sasl-0.4.2/LICENSE
diff --git a/shell/ext-py/thrift_sasl-0.4.1/README.md b/shell/ext-py/thrift_sasl-0.4.2/README.md
similarity index 100%
rename from shell/ext-py/thrift_sasl-0.4.1/README.md
rename to shell/ext-py/thrift_sasl-0.4.2/README.md
diff --git a/shell/ext-py/thrift_sasl-0.4.1/setup.py b/shell/ext-py/thrift_sasl-0.4.2/setup.py
similarity index 98%
rename from shell/ext-py/thrift_sasl-0.4.1/setup.py
rename to shell/ext-py/thrift_sasl-0.4.2/setup.py
index 26c51db..81d8d80 100644
--- a/shell/ext-py/thrift_sasl-0.4.1/setup.py
+++ b/shell/ext-py/thrift_sasl-0.4.2/setup.py
@@ -26,7 +26,7 @@ description = ("Thrift SASL Python module that implements SASL transports for "
 
 setup(
     name='thrift_sasl',
-    version='0.4.1',
+    version='0.4.2',
     description=description,
     long_description=description,
     url='https://github.com/cloudera/thrift_sasl',
diff --git a/shell/ext-py/thrift_sasl-0.4.1/thrift_sasl/__init__.py b/shell/ext-py/thrift_sasl-0.4.2/thrift_sasl/__init__.py
similarity index 94%
rename from shell/ext-py/thrift_sasl-0.4.1/thrift_sasl/__init__.py
rename to shell/ext-py/thrift_sasl-0.4.2/thrift_sasl/__init__.py
index 534b7b2..17fde01 100644
--- a/shell/ext-py/thrift_sasl-0.4.1/thrift_sasl/__init__.py
+++ b/shell/ext-py/thrift_sasl-0.4.2/thrift_sasl/__init__.py
@@ -109,10 +109,10 @@ class TSaslClientTransport(TTransportBase, CReadableTransport):
     self._trans.flush()
 
   def _recv_sasl_message(self):
-    header = self._trans.read(5)
+    header = self._trans_read_all(5)
     status, length = struct.unpack(">BI", header)
     if length > 0:
-      payload = self._trans.read(length)
+      payload = self._trans_read_all(length)
     else:
       payload = ""
     return status, payload
@@ -174,22 +174,29 @@ class TSaslClientTransport(TTransportBase, CReadableTransport):
     return ret + self.__rbuf.read(sz - len(ret))
 
   def _read_frame(self):
-    header = self._trans.read(4)
+    header = self._trans_read_all(4)
     (length,) = struct.unpack(">I", header)
     if self.encode:
       # If the frames are encoded (i.e. you're using a QOP of auth-int or
       # auth-conf), then make sure to include the header in the bytes you send to
       # sasl.decode()
-      encoded = header + self._trans.read(length)
+      encoded = header + self._trans_read_all(length)
       success, decoded = self.sasl.decode(encoded)
       if not success:
         raise TTransportException(type=TTransportException.UNKNOWN,
                                   message=self.sasl.getError())
     else:
       # If the frames are not encoded, just pass it through
-      decoded = self._trans.read(length)
+      decoded = self._trans_read_all(length)
     self.__rbuf = BufferIO(decoded)
 
+  def _trans_read_all(self, sz):
+    try:
+      read_all = self._trans.readAll # Thrift
+    except AttributeError:
+      read_all = self._trans.read # thriftpy
+    return read_all(sz)
+
   def close(self):
     self._trans.close()
     self.sasl = None
diff --git a/shell/packaging/requirements.txt b/shell/packaging/requirements.txt
index cc84d3f..878b2d5 100644
--- a/shell/packaging/requirements.txt
+++ b/shell/packaging/requirements.txt
@@ -5,4 +5,4 @@ setuptools>=36.8.0
 six==1.14.0
 sqlparse==0.1.19
 thrift==0.9.3
-thrift_sasl==0.4.1
+thrift_sasl==0.4.2