You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2023/07/10 08:15:53 UTC
[GitHub] [kvrocks] mapleFU opened a new pull request, #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
mapleFU opened a new pull request, #1574:
URL: https://github.com/apache/kvrocks/pull/1574
Fixes https://github.com/apache/kvrocks/issues/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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] mapleFU commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1629169239
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:
```c++
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`. Would it be ok? @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1629185583
> 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:
>
> ```c++
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1629154905
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] mapleFU commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1628917153
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1630002336
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk merged pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] mapleFU commented on pull request #1574: Style: Change Storage::SetReadOptions to DefaultScanOptions
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1574:
URL: https://github.com/apache/kvrocks/pull/1574#issuecomment-1629215952
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org