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

chore: hset family replies and trim #3748

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dranikpg
Copy link
Contributor

No description provided.

@dranikpg dranikpg force-pushed the hset-fam-trim branch 5 times, most recently from 1879deb to f628bca Compare September 22, 2024 19:45
@dranikpg dranikpg marked this pull request as ready for review September 22, 2024 19:52
dranikpg and others added 2 commits September 22, 2024 22:53
Signed-off-by: Vladislav <vladislav.oleshko@gmail.com>
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
kostasrim
kostasrim previously approved these changes Sep 23, 2024
DCHECK(dynamic_cast<RedisReplyBuilder*>(cntx->reply_builder()));
}

template <typename T> void Send(OpResult<T> res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember you added a similar approach on Sets (although it was not the exact same logic). What I don't remember if you added this on another family. If you did check if there is simlar logic and extract it into a base class which we can the reuse among families with this exact same pattern of replies (if they exist in the first place).

} else {
cntx->SendError(result.status());
}
HSetReplies{cntx}.Send(cntx->transaction->ScheduleSingleHopT(cb));
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here if key not found should return 0 and not key not found
maybe also add a unit test for this and hdel

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see that this logic for hlen is in the OpLen implementation, this is confusing, I would add this logic to OpDel as well instead of the change in line 781

cntx->SendError(result.status());
}
if (result.status() == OpStatus::KEY_NOTFOUND)
result = OpResult<uint32_t>(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add unit test for this please

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.

3 participants