Style: Change Storage::SetReadOptions to DefaultScanOptions

[mapleFU]

Fixes #1572

Now kvrocks has SetWriteOptions and SetReadOptions. However, there syntax doesn't same. SetWriteOptions would set the Storage internal rocksdb::WriteOptions during intialize, and the detail option can be get by DefaultWriteOptions().

However, SetReadOptions only does some fixup when called:

void Storage::SetReadOptions(rocksdb::ReadOptions &read_options) {
  read_options.fill_cache = false;
  read_options.async_io = config_->rocks_db.read_options.async_io;
}

Would we create a DefaultScanOptions, and remove SetReadOptions for that ?

[mapleFU]

@git-hulk By the way, when would kvrocks use fill_cache = true? Seems that only when point get, we would have that, and read_ahead / fill_cache is usally disabled for performance?

[git-hulk]

@git-hulk By the way, when would kvrocks use fill_cache = true? Seems that only when point get,

Yes, only point lookup would enable the fill_cache option.

we would have that, and read_ahead / fill_cache is usally disabled for performance?

Sorry, I didn't get this point. Do you mean that should also enable the fill_cache for the seek operation?

[mapleFU]

No, just make clear when would we we fill_cache, when I'm refactor, I found that some interfaces like ZSet::MGet uses iterator, but it disable metadata cache:

  rocksdb::ReadOptions read_options = storage_->DefaultScanOptions();
  LatestSnapShot ss(storage_);
  read_options.snapshot = ss.GetSnapShot();
  std::string score_bytes, member_key;
  InternalKey(ns_key, member, metadata.version, storage_->IsSlotIdEncoded()).Encode(&member_key);
  s = storage_->Get(read_options, member_key, &score_bytes); //  member key will not be cached.
  if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;

Another case is that Hash::MGet, it uses rocksdb::MGet, but it disable fill_cache. I wonder what kind of get would disable the caching? @git-hulk

[git-hulk]

No, just make clear when would we we fill_cache, when I'm refactor, I found that some interfaces like ZSet::MGet uses iterator, but it disable metadata cache:

  rocksdb::ReadOptions read_options = storage_->DefaultScanOptions();
  LatestSnapShot ss(storage_);
  read_options.snapshot = ss.GetSnapShot();
  std::string score_bytes, member_key;
  InternalKey(ns_key, member, metadata.version, storage_->IsSlotIdEncoded()).Encode(&member_key);
  s = storage_->Get(read_options, member_key, &score_bytes); //  member key will not be cached.
  if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;

Another case is that Hash::MGet, it uses rocksdb::MGet, but it disable fill_cache. I wonder what kind of get would disable the caching? @git-hulk

Got it, thanks for your information. The origin expectation is all point lookups(include mget) should use the fill_cache, so it should be a mistake if we didn't fill the cache in mget operation.

[mapleFU]

The origin expectation is all point lookups(include mget) should use the fill_cache, so it should be a mistake if we didn't fill the cache in mget operation.

So get meta and point get is likely to be a fill_cache, but iterator should always disable cache, because too many sst is included?

Maybe I can make a separate pr for HSet::MGet, this patch just ensure it never modify any syntax, would it be ok?

[git-hulk]

So get meta and point get is likely to be a fill_cache, but iterator should always disable cache, because too many sst is included?

Yes, that's right.

Maybe I can make a separate pr for HSet::MGet, this patch just ensure it never modify any syntax, would it be ok?

Yes, it'd be better to file another PR to make the context more clear.