[SPARK-38178][SS] Correct the logic to measure the memory usage of RocksDB

[Myasuka]

What changes were proposed in this pull request?

Correct the logic to measure the memory usage of RocksDB to include the memory used by block cache.
As "block-cache-pinned-usage" is included in "block-cache-usage", we don't need to sum the pinned usage separately.

Why are the changes needed?

Current reported metrics of RocksDB memory usage is not correct.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

The information could refer to https://github.com/facebook/rocksdb/wiki/Memory-usage-in-RocksDB

[Myasuka]

cc @zsxwing @viirya, please take a look.

[viirya]

based on the doc, do we also need rocksdb.block-cache-pinned-usage?

[dongjoon-hyun]

+1 for @viirya 's comment.

[Myasuka]

based on the doc, do we also need rocksdb.block-cache-pinned-usage?

No, I don't think so, and I actually write my conclusion in original ticket SPARK-38178:
BTW, as the "block-cache-pinned-usage" is included in "block-cache-usage", we don't need to include the pinned usage.

Let's take a look at the RocksDB implementation:

• how methods handle the property query:

  {DB::Properties::kBlockCacheUsage,
   {false, nullptr, &InternalStats::HandleBlockCacheUsage, nullptr,
    nullptr}},
  {DB::Properties::kBlockCachePinnedUsage,
   {false, nullptr, &InternalStats::HandleBlockCachePinnedUsage, nullptr,
    nullptr}},

• And what will call for these two handle methods:

bool InternalStats::HandleBlockCacheUsage(uint64_t* value, DBImpl* /*db*/,
                                          Version* /*version*/) {
  Cache* block_cache;
  bool ok = GetBlockCacheForStats(&block_cache);
  if (!ok) {
    return false;
  }
  *value = static_cast<uint64_t>(block_cache->GetUsage());
  return true;
}

bool InternalStats::HandleBlockCachePinnedUsage(uint64_t* value, DBImpl* /*db*/,
                                                Version* /*version*/) {
  Cache* block_cache;
  bool ok = GetBlockCacheForStats(&block_cache);
  if (!ok) {
    return false;
  }
  *value = static_cast<uint64_t>(block_cache->GetPinnedUsage());
  return true;
}

• And then the implementation of these two getters:

size_t LRUCacheShard::GetUsage() const {
  MutexLock l(&mutex_);
  return usage_;
}

size_t LRUCacheShard::GetPinnedUsage() const {
  MutexLock l(&mutex_);
  assert(usage_ >= lru_usage_);
  return usage_ - lru_usage_;
}

As you can see, the pinned usage is included in the block cache usage.

[dongjoon-hyun]

Thank you for the detail, @Myasuka .

[viirya]

Thank you. Maybe it is good to put a sentence in the PR description to mention the fact that "block-cache-pinned-usage" is included in "block-cache-usage", so we don't need to include it separately.

[Myasuka]

@viirya Thanks for your suggestions, I have already updated the PR description.

[viirya]

Looks like that is something we need to add to RocksDB memory usage.

[HyukjinKwon]

cc @xuanyuanking too FYI

[dongjoon-hyun]

Merged to master/3.2. Thank you, @Myasuka and all.

[dongjoon-hyun]

Welcome to the Apache Spark community, @Myasuka .
You are added to the Apache Spark contributor group too and SPARK-38178 is assigned to you.