Support for the BITFIELD command

[julic20s]

#1896

[mapleFU]

Ooops seems this patch is extremly huge...I need take time for review, just some tiny comments first.

[git-hulk]

@julic20s Can use ./x.py format to format your code before pushing.

[julic20s]

@julic20s Can use ./x.py format to format your code before pushing.

Sorry, I didn't see it at first, I just used shortcut formatting in the ide

[git-hulk]

@julic20s Can use ./x.py format to format your code before pushing.

Sorry, I didn't see it at first, I just used shortcut formatting in the ide

ooh, that's fine. You can also configure clang-format in IDE if supported.

[mapleFU]

Thanks @julic20s . This patch is huge, and I need some time to review the whole patch. I just submit some small comments.

[PragmaTwice]

Thanks for your contribution!

Just some random thoughts without going into the details.

Besides, I believe it would be beneficial to create golang test cases for the recently added command.

[julic20s]

The mutex pool in lock manager may be reallocated by MultiGet(). Then the old mutex pointers in LockGuard all be invalid...I guess that's why I failed the test. Should I use a std::deque to replace the vector?

[git-hulk]

@julic20s Guess not, you should use std::optional<LockGuard> instead of LockGuard directly since the LockGuard constructor will lock the mutex, and if you use LockGuard = default, it won't lock the mutex at all.

refer:

kvrocks/src/types/redis_set.cc

Line 205 in a188009

std::optional<LockGuard> lock_guard;

[julic20s]

Thanks, I get it. The actual cause is that I define a default move assignment function, which cause a dangling pointer.

…

________________________________ 发件人: hulk ***@***.***> 发送时间: 2023年11月22日 16:30 收件人: apache/kvrocks ***@***.***> 抄送: julic20s ***@***.***>; Mention ***@***.***> 主题: Re: [apache/kvrocks] Support for the BITFIELD command (PR #1901) @julic20s<https://github.com/julic20s> Guess not, you should use std::optional<LockGuard> instead of LockGuard directly since the LockGuard constructor will lock the mutex, and if you use LockGuard = default, it won't lock the mutex at all. ― Reply to this email directly, view it on GitHub<#1901 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXIOCT53WTNNJLK4GF5RTLTYFWZ2VAVCNFSM6AAAAAA7TG4R22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSGMYTINRWHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[julic20s]

 LockGuard(LockGuard &&guard) : lock_(guard.lock_) { guard.lock_ = nullptr; } // correct constructor
 LockGuard &operator=(LockGuard &&other) = default; // uncorrect assignment.

😭

[git-hulk]

@julic20s BTW, would you like to take anonymous functions(get/set/get_segment) from Bitmap::bitfield into named functions. This function is a bit hard to read for now.

[mapleFU]

I'm a bit busy on workdays, I'll go through a pass this weekend. If other thinks it's ok we can review an merge it first

[julic20s]

@git-hulk Could you help me approval the workflow? I fix some problems that I found. But I can't reproduction the unittest failed on my local machine. I tried both gcc and clang.

[mapleFU]

done

[julic20s]

I am trying to use rocksdb::PinnableSlice to improve Bitmap::SegmentCacheStore. But I failed because of some strange error. When I use std::unordered_map<uint32_t, rocksdb::PinnableSlice> cache_ and get its iterator, the iterator compare function is failed to instantiate (such as: it != cache_.end()), which makes me can not travel cache_ in BatchForFlush(). rocksdb::Cleanable has a bug in current rocksdb version(It's copy constructor deleted, but the param of it is not marked const).

[PragmaTwice]

I am trying to use rocksdb::PinnableSlice to improve Bitmap::SegmentCacheStore. But I failed because of some strange error. When I use std::unordered_map<uint32_t, rocksdb::PinnableSlice> cache_ and get its iterator, the iterator compare function is failed to instantiate (such as: it != cache_.end()), which makes me can not travel cache_ in BatchForFlush(). rocksdb::Cleanable has a bug in current rocksdb version(It's copy constructor deleted, but the param of it is not marked const).

I don't think using PinnableSlice is necessary for this PR to be merged.

[julic20s]

@mapleFU Hi, I finished all changes you pointed, could you review them again? Thanks.

[mapleFU]

Sure, I'm now becoming very busy on workdays, I'll finish review these few days.

[git-hulk]

@PragmaTwice @mapleFU I think we can go ahead to merge this PR if overall looks good.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

No Coverage information
0.5% Duplication

[PragmaTwice]

Thank you for your contribution!