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 2024/02/23 05:54:33 UTC
[PR] Support negative count during Set::Take [kvrocks]
mapleFU opened a new pull request, #2113:
URL: https://github.com/apache/kvrocks/pull/2113
This patch:
1. Support negative count during Set::Take. Note that I just not return the duplicate key
2. Not call Write during srandmember
--
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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#issuecomment-1962277259
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2113) **Quality Gate failed**
Failed conditions
 [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2113&resolved=false&inNewCodePeriod=true)
 [60.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2113&metric=new_coverage&view=list) (required ≥ 60%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2113)
--
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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113
--
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
Re: [PR] Support negative count during Set::Take [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#discussion_r1500246071
##########
src/types/redis_set.cc:
##########
@@ -197,9 +197,14 @@ rocksdb::Status Set::MIsMember(const Slice &user_key, const std::vector<Slice> &
}
rocksdb::Status Set::Take(const Slice &user_key, std::vector<std::string> *members, int count, bool pop) {
- int n = 0;
members->clear();
- if (count <= 0) return rocksdb::Status::OK();
+ if (count == 0) return rocksdb::Status::OK();
+ if (count < 0) {
+ DCHECK(!pop);
+ // NOTE: Currently, for SRANDMEMBER, we don't
+ // make duplicate members to be returned.
Review Comment:
Aha, let me extract a common function to support it 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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#issuecomment-1962267575
cc @git-hulk @PragmaTwice
--
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
Re: [PR] Support negative count during Set::Take [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#discussion_r1500246071
##########
src/types/redis_set.cc:
##########
@@ -197,9 +197,14 @@ rocksdb::Status Set::MIsMember(const Slice &user_key, const std::vector<Slice> &
}
rocksdb::Status Set::Take(const Slice &user_key, std::vector<std::string> *members, int count, bool pop) {
- int n = 0;
members->clear();
- if (count <= 0) return rocksdb::Status::OK();
+ if (count == 0) return rocksdb::Status::OK();
+ if (count < 0) {
+ DCHECK(!pop);
+ // NOTE: Currently, for SRANDMEMBER, we don't
+ // make duplicate members to be returned.
Review Comment:
Aha, let me extract a common function 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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#issuecomment-1961889303
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2113) **Quality Gate failed**
Failed conditions
 [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2113&resolved=false&inNewCodePeriod=true)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2113)
--
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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#discussion_r1501046133
##########
src/types/redis_set.cc:
##########
@@ -210,49 +215,30 @@ rocksdb::Status Set::Take(const Slice &user_key, std::vector<std::string> *membe
rocksdb::Status s = GetMetadata(ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
- auto batch = storage_->GetWriteBatchBase();
- WriteBatchLogData log_data(kRedisSet);
- batch->PutLogData(log_data.Encode());
-
- std::string prefix = InternalKey(ns_key, "", metadata.version, storage_->IsSlotIdEncoded()).Encode();
- std::string next_version_prefix = InternalKey(ns_key, "", metadata.version + 1, storage_->IsSlotIdEncoded()).Encode();
-
- rocksdb::ReadOptions read_options = storage_->DefaultScanOptions();
- LatestSnapShot ss(storage_);
- read_options.snapshot = ss.GetSnapShot();
- rocksdb::Slice upper_bound(next_version_prefix);
- read_options.iterate_upper_bound = &upper_bound;
-
- std::vector<std::string> iter_keys;
- iter_keys.reserve(count);
- std::random_device rd;
- std::mt19937 gen(rd());
- auto iter = util::UniqueIterator(storage_, read_options);
- for (iter->Seek(prefix); iter->Valid() && iter->key().starts_with(prefix); iter->Next()) {
- ++n;
- if (n <= count) {
- iter_keys.push_back(iter->key().ToString());
- } else { // n > count
- std::uniform_int_distribution<> distrib(0, n - 1);
- int random = distrib(gen); // [0,n-1]
- if (random < count) {
- iter_keys[random] = iter->key().ToString();
- }
- }
+ ObserverOrUniquePtr<rocksdb::WriteBatchBase> batch = storage_->GetWriteBatchBase();
+ if (pop) {
+ WriteBatchLogData log_data(kRedisSet);
+ batch->PutLogData(log_data.Encode());
}
- for (Slice key : iter_keys) {
- InternalKey ikey(key, storage_->IsSlotIdEncoded());
- members->emplace_back(ikey.GetSubKey().ToString());
- if (pop) {
- batch->Delete(key);
- }
+ members->clear();
+ s = ExtractRandMemberFromSet<std::string>(
+ unique, count, [this, user_key](std::vector<std::string> *samples) { return this->Members(user_key, samples); },
+ members);
+ if (!s.ok()) {
+ return s;
}
- if (pop && !iter_keys.empty()) {
- metadata.size -= iter_keys.size();
- std::string bytes;
- metadata.Encode(&bytes);
- batch->Put(metadata_cf_handle_, ns_key, bytes);
+ // Avoid to write an empty op-log if just random select some members.
+ if (!pop) return rocksdb::Status::OK();
+ // Avoid to write an empty op-log if the set is empty.
Review Comment:
Don't know whether we can do this optimization, would it affect replication?
--
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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#issuecomment-1964078694
> LGTM. I just wondering whether @PragmaTwice will advise to rename `sample_helper.h` to something with `_util.h` suffix like `random_util/random_sample_util/sample_util` :)
I am fine since this file is not in common dir.
--
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
Re: [PR] Extract a rand sample helper and support negative sample count in "Set" [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#discussion_r1501047230
##########
src/types/redis_hash.cc:
##########
@@ -389,43 +390,30 @@ rocksdb::Status Hash::RandField(const Slice &user_key, int64_t command_count, st
rocksdb::Status s = GetMetadata(ns_key, &metadata);
if (!s.ok()) return s;
- uint64_t size = metadata.size;
std::vector<FieldValue> samples;
// TODO: Getting all values in Hash might be heavy, consider lazy-loading these values later
if (count == 0) return rocksdb::Status::OK();
- s = GetAll(user_key, &samples, type);
- if (!s.ok()) return s;
- auto append_field_with_index = [field_values, &samples, type](uint64_t index) {
- if (type == HashFetchType::kAll) {
- field_values->emplace_back(samples[index].field, samples[index].value);
- } else {
- field_values->emplace_back(samples[index].field, "");
- }
- };
- field_values->reserve(std::min(size, count));
- if (!unique || count == 1) {
- // Case 1: Negative count, randomly select elements or without parameter
- std::mt19937 gen(std::random_device{}());
- std::uniform_int_distribution<uint64_t> dis(0, size - 1);
- for (uint64_t i = 0; i < count; i++) {
- uint64_t index = dis(gen);
- append_field_with_index(index);
- }
- } else if (size <= count) {
- // Case 2: Requested count is greater than or equal to the number of elements inside the hash
- for (uint64_t i = 0; i < size; i++) {
- append_field_with_index(i);
- }
- } else {
- // Case 3: Requested count is less than the number of elements inside the hash
- std::vector<uint64_t> indices(size);
- std::iota(indices.begin(), indices.end(), 0);
- std::mt19937 gen(std::random_device{}());
- std::shuffle(indices.begin(), indices.end(), gen); // use Fisher-Yates shuffle algorithm to randomize the order
- for (uint64_t i = 0; i < count; i++) {
- uint64_t index = indices[i];
- append_field_with_index(index);
+ s = ExtractRandMemberFromSet<FieldValue>(
+ unique, count,
+ [this, user_key, type](std::vector<FieldValue> *elements) { return this->GetAll(user_key, elements, type); },
+ field_values);
+ if (!s.ok()) {
+ return s;
+ }
+ switch (type) {
Review Comment:
This is just a dcheck. Previously, code called with `append_field_with_index`. However, `GetAll` is already called with "type", so it's not neccessary to clean up or set it again
--
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
Re: [PR] Support negative count during Set::Take [kvrocks]
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #2113:
URL: https://github.com/apache/kvrocks/pull/2113#discussion_r1500245526
##########
src/types/redis_set.cc:
##########
@@ -197,9 +197,14 @@ rocksdb::Status Set::MIsMember(const Slice &user_key, const std::vector<Slice> &
}
rocksdb::Status Set::Take(const Slice &user_key, std::vector<std::string> *members, int count, bool pop) {
- int n = 0;
members->clear();
- if (count <= 0) return rocksdb::Status::OK();
+ if (count == 0) return rocksdb::Status::OK();
+ if (count < 0) {
+ DCHECK(!pop);
+ // NOTE: Currently, for SRANDMEMBER, we don't
+ // make duplicate members to be returned.
Review Comment:
`Currently, we don't support the returning of the same element multiple times` or something like that because `duplicate members` can be confusing since set stores only unique elements.
--
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