tutububug commented on PR #2142: URL: https://github.com/apache/kvrocks/pull/2142#issuecomment-1986918990
> Regarding your design, I have some questions: > > 1. Based on the current design, typically one Redis key will introduce a maximum of 16384 RocksDB keys (registers). Each value corresponding to a RocksDB key contains only one integer. This may be inefficient; merging multiple registers onto one key could reduce the number of keys introduced. WDYT? Not really. The register(subkey) only be stored which its count is not zero. This point is different from the memory implementation with static array as dense encode. On disk, I think its sparse encode naturally. > > 2. I noticed that integers are stored as string representations (`std::to_string`) rather than their binary form (e.g., subkey and register value). What is the reason for this approach? The number of consecutive 0s is calculated from the last 50 digits of the hash value, so the maximum value is 50, and the maximum value stored in a string is 2 bytes. It should not waste a lot of space, and at the same time save the overhead of integer encoding and decoding. For keys, it may be more efficient, but the largest index is only 5 bytes (16383). > > 3. Having a constant `size` seems illogical since the number of subkeys linked to this Redis key varies. (However, if every write operation modifies the metadata, it may lead to a decrease in performance. I don't have a clear idea about this aspect.) Currently, the ‘size’ variable has no practical purpose; the only requirement is that it be non-zero. Due to the implementation of kvrocks getmetadata, non-string type data structures with a size of 0 are judged to be expired, and the HLL add parameter that redis has implemented allows no parameters but the key will be stored. For compatibility, size is used as a constant to prevent expiration. > > > Concerning the code, although I haven't reviewed it thoroughly yet, there are some points worth mentioning: > > 1. The complete source code of MurmurHash can be placed in the `vendor` directory. OK. > > 2. It appears that using `PFADD` in the code leads to an increasing number of RocksDB keys (registers). There seems to be no operation that reduces these keys until deleting this Redis key. How can we prevent an increase in RocksDB keys without a mechanism to decrease them? For an HLL user key, the maximum register value is 16384, and it cannot be larger. In fact, I think this should be considered controllable compared to data structures such as hash, set, list, etc. whose size is determined by user input. > > @PragmaTwice cc @git-hulk @mapleFU -- 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]
