mapleFU commented on code in PR #1699:
URL: https://github.com/apache/kvrocks/pull/1699#discussion_r1305098218
##########
src/storage/redis_metadata.h:
##########
@@ -198,3 +200,27 @@ class StreamMetadata : public Metadata {
void Encode(std::string *dst) override;
rocksdb::Status Decode(const std::string &bytes) override;
};
+
+class SBChainMetadata : public Metadata {
+ public:
+ uint16_t n_filters;
Review Comment:
Though personally I understand what these arguments means. But I think we
can add comments here.
We can copy paste some comments from
https://redis.io/docs/data-types/probabilistic/bloom-filter/ . Would can make
reviewer understand more about the syntax here. Like:
```c++
/// number of child Bloom Filters
uint16_t n_filters;
/// Adding an element to a Bloom filter never fails due to the data
structure "filling up". Instead the error rate
/// starts to grow. To keep the error close to the one set on filter
initialisation - the bloom filter will auto-scale,
/// meaning when capacity is reached an additional sub-filter will be
created.
///
/// The size of the new sub-filter is the size of the last sub-filter
multiplied by EXPANSION.
///
/// The default expansion value is 2.
///
/// For non-scaling, the value should be...
uint16_t scaling;
uint16_t expansion;
```
And by the way, we should:
1. if no scaling, define a macro or constant for it, and define what
`expansion` should be in this case
2. Try to simplify two argument into just one?
##########
src/storage/redis_metadata.cc:
##########
@@ -401,3 +401,73 @@ rocksdb::Status StreamMetadata::Decode(const std::string
&bytes) {
return rocksdb::Status::OK();
}
+
+void SBChainMetadata::Encode(std::string *dst) {
+ Metadata::Encode(dst);
+
+ PutFixed16(dst, n_filters);
+ PutFixed16(dst, scaling);
+ PutFixed16(dst, expansion);
+}
+
+rocksdb::Status SBChainMetadata::Decode(const std::string &bytes) {
+ Slice input(bytes);
+ if (!GetFixed8(&input, &flags)) {
Review Comment:
@PragmaTwice Can it be like:
```c++
Status s = Metadata::Decode(bytes);
```
To avoid copy-paste original `Metadata::Decode`'s code?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
