You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/20 12:18:00 UTC
[GitHub] [incubator-kvrocks] tufitko opened a new pull request, #902: feat: impl FilterBlobByKey for SubKeyFilter
tufitko opened a new pull request, #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902
#886
implement FilterBlobByKey to avoid blobs reading in compactions when using BlobDB
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976416023
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
Oh, okey, it’s good, I will be commited later
--
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] [incubator-kvrocks] git-hulk commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976027388
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
We can remove all debug logs in compaction filter since it will affect the compaction performance even it's disabled in production.
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976417368
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
Good, thanks
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r975959389
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
I think this log content should be distinguished with the log in Filter (line 119), although the location of the log can be determined by the line of the file.
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
I don't think the name `GetMetadata` is clear, so I'm confused here.
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976408829
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
`FilterBlobByKey()` will be executed before `Filter()`, so I think `FilterBlobByKey()` should be declared and defined before `Filter()`.
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976307295
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
So it does more than one thing, and we should separate those things.
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976365605
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
Hm, I don't know how I can split it.
there are 4 actions:
- get_db
- build_metadata_key
- find_metdata
- decode_metadata
And all of the actions need to get metadata..
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976418011
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
Yeah, it's not a big deal, but it's more logical :)
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976014473
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
`[compact_filter/subkey_blob]` is it 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
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976038670
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
Let me think about it. I still don't think the logic here is clear enough.
--
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] [incubator-kvrocks] git-hulk commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976508152
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
❤️ Thanks
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976408829
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
`IsMetadataExpired()` will be executed before `Filter()`, so I think `IsMetadataExpired()` should be declared and defined before `Filter()`.
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
> @tufitko @caipengbo I think another idea is GetMetadata returns Status instead of bool, so that caller determine whether to keep the subkey/blob or not. Not sure if I clarify clearly enough.
Yeah, that makes it clearer!
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
I thought about it a little more, `GetMetadata` is 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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976407745
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
The pseudo-code should be like below:
```
Filter() {
Status s = GetMetadata(ikey, &metadata);
if (s.IsNotFound()) {
return true;
}
if (!s.IsOK()) {
// log error
return false;
}
... check is expired or other logic here
}
```
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r975959389
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
I think this log content should be distinguished with the log in Filter (line 119), although the location of the log can be determined by the line of the file and even though it is a Debug log.
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r975974445
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
I didn't invent a better name. `FindMetadata`, `FillMetadata`?
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976416620
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
Ok, it’s no problem
--
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] [incubator-kvrocks] tufitko commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976127923
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
this function is looking for metadata and fills it, also returned true if kv can be deleted.
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
okey
--
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] [incubator-kvrocks] caipengbo commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976034464
##########
src/compact_filter.cc:
##########
@@ -117,4 +124,27 @@ bool SubKeyFilter::Filter(int level,
<< ", result: " << (result ? "deleted" : "reserved");
return result;
}
+
+rocksdb::CompactionFilter::Decision SubKeyFilter::FilterBlobByKey(int level,
+ const Slice& key, std::string* new_value, std::string* skip_until) const {
+ InternalKey ikey(key, stor_->IsSlotIdEncoded());
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return rocksdb::CompactionFilter::Decision::kRemove;
+ }
+ // bitmap will be checked in Filter
+ if (metadata.Type() == kRedisBitmap) {
+ return rocksdb::CompactionFilter::Decision::kUndetermined;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata);
+ DLOG(INFO) << "[compact_filter/subkey] "
+ << "namespace: " << ikey.GetNamespace().ToString()
+ << ", metadata key: " << ikey.GetKey().ToString()
+ << ", subkey: " << ikey.GetSubKey().ToString()
+ << ", verison: " << ikey.GetVersion()
+ << ", result: " << (result ? "deleted" : "reserved");
Review Comment:
Agree with it.
--
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] [incubator-kvrocks] tanruixiang commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976392196
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
```suggestion
rocksdb::CompactionFilter::Decision FilterBlobByKey(int level, const Slice& key,
std::string* new_value,
std::string* skip_until) const override;
```
I think we should unify the style here.
--
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] [incubator-kvrocks] git-hulk commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976404521
##########
src/compact_filter.cc:
##########
@@ -108,7 +110,12 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
+ Metadata metadata(kRedisNone, false);
+ if (GetMetadata(ikey, &metadata)) {
+ return true;
+ }
Review Comment:
@tufitko @caipengbo I think another idea is `GetMetadata` returns Status instead of bool, so that caller determine whether to keep the subkey/blob or not. Not sure if I clarify clearly enough.
--
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] [incubator-kvrocks] tanruixiang commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r976392196
##########
src/compact_filter.h:
##########
@@ -64,9 +64,13 @@ class SubKeyFilter : public rocksdb::CompactionFilter {
stor_(storage) {}
const char *Name() const override { return "SubkeyFilter"; }
- bool IsKeyExpired(const InternalKey &ikey, const Slice &value) const;
+ bool GetMetadata(const InternalKey &ikey, Metadata* metadata) const;
+ bool IsMetadataExpired(const InternalKey &ikey, const Metadata& metadata) const;
bool Filter(int level, const Slice &key, const Slice &value,
std::string *new_value, bool *modified) const override;
+ rocksdb::CompactionFilter::Decision FilterBlobByKey(int /*level*/, const Slice& /*key*/,
+ std::string* /*new_value*/,
+ std::string* /*skip_until*/) const override;
Review Comment:
```suggestion
rocksdb::CompactionFilter::Decision FilterBlobByKey(int level, const Slice &key,
std::string *new_value,
std::string *skip_until) const override;
```
I think we should unify the style here.
--
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] [incubator-kvrocks] git-hulk merged pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902
--
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] [incubator-kvrocks] git-hulk commented on a diff in pull request #902: feat: impl FilterBlobByKey for SubKeyFilter
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #902:
URL: https://github.com/apache/incubator-kvrocks/pull/902#discussion_r977622047
##########
src/compact_filter.cc:
##########
@@ -108,13 +127,22 @@ bool SubKeyFilter::Filter(int level,
std::string *new_value,
bool *modified) const {
InternalKey ikey(key, stor_->IsSlotIdEncoded());
- bool result = IsKeyExpired(ikey, value);
- DLOG(INFO) << "[compact_filter/subkey] "
- << "namespace: " << ikey.GetNamespace().ToString()
- << ", metadata key: " << ikey.GetKey().ToString()
- << ", subkey: " << ikey.GetSubKey().ToString()
- << ", verison: " << ikey.GetVersion()
- << ", result: " << (result ? "deleted" : "reserved");
+ Metadata metadata(kRedisNone, false);
+ Status s = GetMetadata(ikey, &metadata);
+ if (s.Is<Status::NotFound>()) {
+ return true;
+ }
+ if (!s.IsOK()) {
+ LOG(ERROR) << "[compact_filter/subkey] Failed to get metadata"
+ << ", namespace: " << ikey.GetNamespace().ToString()
+ << ", key: " << ikey.GetKey().ToString()
+ << ", err: " << s.Msg();
+ return false;
+ }
+
+ bool result = IsMetadataExpired(ikey, metadata) ||
+ (metadata.Type() == kRedisBitmap && Redis::Bitmap::IsEmptySegment(value));
return result;
Review Comment:
```suggestion
return IsMetadataExpired(ikey, metadata) ||
(metadata.Type() == kRedisBitmap && Redis::Bitmap::IsEmptySegment(value));
```
--
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