Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cluster): impl slot data delete #1224

Merged
merged 3 commits into from May 17, 2023
Merged

feat(cluster): impl slot data delete #1224

merged 3 commits into from May 17, 2023

Conversation

adiholden
Copy link
Collaborator

@adiholden adiholden commented May 16, 2023

I created FlushSlots function in db_slice. It should be called from DFLY CLUSTER CONFIG command when slots are deleted from node.

Signed-off-by: adi_holden <adi@dragonflydb.io>
@adiholden adiholden requested a review from chakaz May 16, 2023 13:29
chakaz
chakaz previously approved these changes May 16, 2023
@@ -495,6 +495,43 @@ bool DbSlice::Del(DbIndex db_ind, PrimeIterator it) {
return true;
}

void DbSlice::FlushSlotsFb(const absl::flat_hash_set<SlotId>& slot_ids) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If flat_hash_set<SlotId> is being passed a lot I recommend introducing an alias

uint64_t next_version = NextVersion();

auto del_entry_cb = [&](PrimeTable::iterator it) {
std::string tmp;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull tmp out of the callback - to reuse it through all the iteration sequence.

Signed-off-by: adi_holden <adi@dragonflydb.io>
@adiholden
Copy link
Collaborator Author

@romange @chakaz please take a look on the second commit. I understood that I have problem with the lifetime of slots_set

mi_heap_collect(ServerState::tlocal()->data_heap(), true);
}

void DbSlice::FlushSlots(const SlotSet&& slot_ids) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you do need const I guess

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a const T&& is something that should not exist.
If you want to take ownership over a parameter, just take it by value (i.e. FlushSlots(SlotSet slot_ids)). That will make it possible to call this function both with rvalues (which will be efficient and will duck an unnecessary copy), or with an lvalue (meaning just a regular variable), which will copy but will not be less efficient than accepting a const T&.
By accepting a T&& argument, you are forcing the caller to pass in a temporary, i.e. this code won't work:

SlotSet slots;
// ...
FlushSlots(slots)

So only use that convention to force callers not to copy.

Signed-off-by: adi_holden <adi@dragonflydb.io>
@adiholden adiholden requested a review from chakaz May 17, 2023 08:38
@adiholden adiholden merged commit f96a083 into main May 17, 2023
6 checks passed
@romange romange deleted the delete_slot_data branch May 21, 2023 01:12
romange pushed a commit that referenced this pull request Jun 1, 2023
* feat(cluster): impl slot data delete

Signed-off-by: adi_holden <adi@dragonflydb.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants