feat: add HEXPIRE and FIELDEXPIRE #3842

NegatioN · 2024-10-01T11:00:09Z

Continuation of: #3780
To implement issue: #3027
Here's my suggestion for how to add the FIELDEXPIRE and HEXPIRE commands. I tried to split it up similarly to how FieldTtl does generic operations across set_family and hset_family.

I've not added support for the optional prefixes [NX | XX | GT | LT], as I see few other commands do it, and I also don't really need it. (Can probably be further work?)

Responses from both commands are (per element queried): -2 for non-existing key/field, 0 if ttl is 0, and ttl gets set, normal success=1

I have a few questions at this point:

I might need help with the flags that are set for the given commands. I frankly just set something that seems to work, and compile.
I'm also unsure about the acl-settings for the commands.

Open for comments. Tagging @romange @kostasrim, since you're already familiar with the previous PR.

kostasrim

Hi @NegatioN and thaaaank you for your contribution!

I left some comments mostly about adding a couple of test cases, some corner cases and some nits to avoid code duplication :)

Thank you again 🚀 💯

src/server/hset_family.cc

src/server/generic_family.cc

src/facade/cmd_arg_parser.h

src/server/generic_family.cc

src/server/hset_family.cc

src/server/hset_family_test.cc

src/server/hset_family.cc

src/server/generic_family.cc

kostasrim · 2024-10-02T14:01:18Z

@NegatioN let me know when you addressed all the comments and I will rereview :)

src/core/compact_object.h

romange · 2024-10-03T09:46:09Z

src/server/set_family.h

@@ -33,6 +33,9 @@ class SetFamily {
  static int32_t FieldExpireTime(const DbContext& db_context, const PrimeValue& pv,
                                 std::string_view field);

+  static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, PrimeValue& pv,


our style guide requires that we pass output variables (pv) last and by pointer.

romange · 2024-10-03T09:46:31Z

src/server/hset_family.h

@@ -29,9 +29,14 @@ class HSetFamily {
  static int32_t FieldExpireTime(const DbContext& db_context, const PrimeValue& pv,
                                 std::string_view field);

+  static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, PrimeValue& pv,


our style guide requires that we pass output variables (pv) last and by pointer.

Just to clarify, the signature should be like this?

static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, uint32_t ttl_sec, std::string_view key, CmdArgList values, PrimeValue& pv)

I think he wants:

static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, uint32_t ttl_sec, std::string_view key, CmdArgList values, PrimeValue* pv)

Notice last argument, it's not a reference but a pointer. (That's from our style guide)

Also @NegatioN I am fine with the PR once this is addressed. The rest of my comments are NIT.

LGTM! 🚀

NegatioN · 2024-10-03T10:32:14Z

@kostasrim I think this is ready for a review again now.

kostasrim · 2024-10-03T11:38:58Z

src/server/family_utils.h

+template <typename O>
+static std::vector<long> ExpireElements(void* ptr, const facade::CmdArgList values,
+                                        uint32_t ttl_sec) {
+  O* owner = (O*)ptr;
+  std::vector<long> res;


Suggested change

template <typename O>

static std::vector<long> ExpireElements(void* ptr, const facade::CmdArgList values,

uint32_t ttl_sec) {

O* owner = (O*)ptr;

std::vector<long> res;

template <typename DenseSet>

static std::vector<long> ExpireElements(DenseSet* owner, const facade::CmdArgList values,

uint32_t ttl_sec) {

std::vector<long> res;

that way you don't need:

To cast a void* to the respective type

You can now call ExpireElements(arg1, arg2) without the <> syntax. (that is, let the compiler automatically deduce this)

kostasrim · 2024-10-03T11:43:43Z

src/server/hset_family.h

@@ -29,9 +29,14 @@ class HSetFamily {
  static int32_t FieldExpireTime(const DbContext& db_context, const PrimeValue& pv,
                                 std::string_view field);

+  static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, PrimeValue& pv,


I think he wants:

static std::vector<long> SetFieldsExpireTime(const OpArgs& op_args, uint32_t ttl_sec, std::string_view key, CmdArgList values, PrimeValue* pv)

Notice last argument, it's not a reference but a pointer. (That's from our style guide)

kostasrim · 2024-10-03T11:44:08Z

src/server/generic_family.cc

+
+  OpResult<vector<long>> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
+  auto* rb = static_cast<RedisReplyBuilder*>(cntx->reply_builder());
+  if (result) {


This could be a function but its fine :)

NegatioN · 2024-10-03T12:38:27Z

@kostasrim Done

kostasrim · 2024-10-03T13:51:47Z

LGTM and thank you for the contribution @NegatioN

NegatioN · 2024-10-04T09:33:55Z

btw I have no access to merge the PR, so someone else has to do that for me @kostasrim :)
I want to say thanks for the good feedback from both of you as well, and for staying patient with me even though I have near zero c++ experience.
It's been a fun exercise :)

romange · 2024-10-04T14:49:47Z

I must say it was a pretty good PR for Zero C++ experience 😮 😮 😅

* add hexpire * add fieldexpire * add tests

romange added the hacktoberfest-accepted label Oct 1, 2024

kostasrim self-requested a review October 1, 2024 15:52

kostasrim reviewed Oct 1, 2024

View reviewed changes

NegatioN commented Oct 2, 2024

View reviewed changes

src/server/generic_family.cc Show resolved Hide resolved

romange reviewed Oct 3, 2024

View reviewed changes

src/core/compact_object.h Outdated Show resolved Hide resolved

romange reviewed Oct 3, 2024

View reviewed changes

src/core/compact_object.h Outdated Show resolved Hide resolved

romange reviewed Oct 3, 2024

View reviewed changes

NegatioN added 6 commits October 3, 2024 12:00

feat: add HEXPIRE and FIELDEXPIRE

2814994

fix: minor adjustments

34ebd95

chore: make CompactObj::ExpireElements template

9f77722

chore: add some testcases

e75db1a

chore: move template function

e239803

chore: touch-ups

bd88ad0

kostasrim reviewed Oct 3, 2024

View reviewed changes

NegatioN added 2 commits October 3, 2024 14:31

chore: change pv to pointer type

1a49875

chore: Make template take in DenseSet

3933f8b

kostasrim approved these changes Oct 3, 2024

View reviewed changes

Merge branch 'main' into feat/fieldexpire-hashexpire

0538772

kostasrim merged commit 0f972a0 into dragonflydb:main Oct 4, 2024
9 checks passed

kostasrim pushed a commit that referenced this pull request Oct 7, 2024

feat: add HEXPIRE and FIELDEXPIRE (#3842)

b87227a

* add hexpire * add fieldexpire * add tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add HEXPIRE and FIELDEXPIRE #3842

feat: add HEXPIRE and FIELDEXPIRE #3842

NegatioN commented Oct 1, 2024

kostasrim left a comment

kostasrim commented Oct 2, 2024

romange Oct 3, 2024

romange Oct 3, 2024

NegatioN Oct 3, 2024 •

edited

Loading

kostasrim Oct 3, 2024 •

edited

Loading

kostasrim Oct 3, 2024

NegatioN commented Oct 3, 2024

kostasrim Oct 3, 2024

kostasrim Oct 3, 2024

kostasrim Oct 3, 2024 •

edited

Loading

kostasrim Oct 3, 2024

NegatioN commented Oct 3, 2024

kostasrim commented Oct 3, 2024

NegatioN commented Oct 4, 2024 •

edited

Loading

romange commented Oct 4, 2024

feat: add HEXPIRE and FIELDEXPIRE #3842

feat: add HEXPIRE and FIELDEXPIRE #3842

Conversation

NegatioN commented Oct 1, 2024

kostasrim left a comment

Choose a reason for hiding this comment

kostasrim commented Oct 2, 2024

romange Oct 3, 2024

Choose a reason for hiding this comment

romange Oct 3, 2024

Choose a reason for hiding this comment

NegatioN Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

kostasrim Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

kostasrim Oct 3, 2024

Choose a reason for hiding this comment

NegatioN commented Oct 3, 2024

kostasrim Oct 3, 2024

Choose a reason for hiding this comment

kostasrim Oct 3, 2024

Choose a reason for hiding this comment

kostasrim Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

kostasrim Oct 3, 2024

Choose a reason for hiding this comment

NegatioN commented Oct 3, 2024

kostasrim commented Oct 3, 2024

NegatioN commented Oct 4, 2024 • edited Loading

romange commented Oct 4, 2024

NegatioN Oct 3, 2024 •

edited

Loading

kostasrim Oct 3, 2024 •

edited

Loading

kostasrim Oct 3, 2024 •

edited

Loading

NegatioN commented Oct 4, 2024 •

edited

Loading