feat(hset_family): Add KEEPTTL support to HSetEx

[abhijat]

The KEEPTTL option is added to HSetEx command.

This option if specified makes sure that TTL is preserved for existing members. It coexists along with the NX option and should be specified at the start of the command:

HSETEX key [NX] [KEEPTTL] seconds field value [field value ...]

fixes #4701

[kostasrim]

Maybe I am missing something but I think we don't really need two searches in any case since we can update the ttl field of dptr directly. So why can't we simply do:

if (sds prev_entry = (sds)AddOrReplaceObj(newkey, sdsval_tag & kValTtlBit, dptr); prev_entry) {
  // We know that there was a `prev_entry` so `keepttl` is relevant here. 
  if(keepttl) {
    auto old_ttl = ObjExpireTime(prev_entry);
    if(old_ttl != UINT_MAX) {
      ObjUpdateExpireTime(dptr, old_ttl);
    }
  }
}

[kostasrim]

Unless I am missing something then:

1. We don't need this search pass what found to AddOrReplaceObj dance.
2. It works because HSETEX will always add a ttl field so CreateEntry above will always "reserve" space for the ttl field anyway

[kostasrim]

all in all we can avoid this alltogether basically

Passing the result of search to AddOrReplaceObj as currently implemented in this PR to avoid two lookups

So there should be no extra overhead associated with the two searches

[abhijat]

To update the ttl of dptr after adding the object as you proposed, we need it to be initialized and pointing to the set member.

But we only get it from the first search added in this PR at

dragonfly/src/core/string_map.cc

Line 88 in 328c96b

std::tie(dptr, raw) = FindObjPtr(&field, hashcode, 1);

If you compare with the main branch, we did not originally have access to dptr in this method, so it wouldn't be possible to update the ttl without the first search.

I had also thought of adding a call to ObjUpdateExpireTime inside DenseSet::AddOrReplaceObj after we find the prev. entry, this way there is only one search and the code is much simpler (a flag has to be passed into AddOrReplaceObj to preserve ttl), something like the following in DenseSet::AddOrReplaceObj:

void* res = dptr->Raw();
if (has_ttl && keepttl) {
  if (const auto ttl = ObjExpireTime(res); ttl != UINT32_MAX) {
    ObjUpdateExpireTime(res, ttl);
  }  
}

but that will change the behavior for all subclasses of DenseSet instead of just the string map class. Maybe the keepttl flag can be false by default so it doesn't change the behavior of other subclasses.

[kostasrim]

dptr was a typo. You don't need the denseptr to update the object. I will ping to explain internally.

[abhijat]

made changes as discussed

[kostasrim]

Do we really need all of this ? Why not bool skip_if_exists = static_cast<bool>(parser.Check("KEEPTTL"sv)); similarly to what we have on line 729 ? (see skip_if_exists)

[abhijat]

I originally tried with Check but it seems to only work for a fixed order, eg if we have:

bool keepttl = parser.Check("KEEPTTL"sv);
bool skip = parser.Check("NX"sv);

This works if

• only KEEPTTL is passed
• only NX is passed
• both are passed in the exact order KEEPTTL NX

But it rejects the command if we call HSETEX key NX KEEPTTL ..., so we have the order hardcoded.

If we reverse the order of parser.Check calls then NX KEEPTTL will work but KEEPTTL NX will not work.

So I used a loop so the current version of the code handles the options in any order.

[abhijat]

If the order of options is guaranteed to be fixed eg NX is always before KEEPTTL then I can add the two parser.Check calls in the same order.

[abhijat]

switched to use Check in a loop as discussed

[kostasrim]

Thinking about this again why don't we just enforce an order ? NX should go first, followed by KEEPTTL if it exists.

So we rewrite this as:

if (parser.Check("NX")) {
  op_sp.skip_i_exists = true;
}
if (parser.Check("KEEPTTL")) {
      op_sp.keepttl = true;
}

And we are done.

[abhijat]

change to something similar

[adiholden]

I dont think this is a good decision to enforce the order. We dont do this in other commands afaik. This is actually more of a product decision of to enforce order of commands.
Please create a fix PR to not enforce the order @abhijat

[kostasrim]

Before we advance time it would worth to assert that:

EXPECT_EQ(Run({"FIELDTTL", "k", "kttlfield"}).GetInt(), long_time);
EXPECT_EQ(Run({"FIELDTTL", "k", "afield"}).GetInt(), long_time);

[abhijat]

Added. Usually I'm a little wary of adding assertions where we check exact time values, but in this case I guess the test runs fast enough so it wouldn't fail.

[kostasrim]

If we don't advance time, it will remain the same. We have full control over this :)

[kostasrim]

LGTM 🚢 🇮🇹 👨‍🍳

[kostasrim]

I see. Ouch. We had a DCHECK in ObjExpireTime which we triggered right ? And then, ObjUpdateExpireTime used now() which changed the TTL. IMO I think the DCHECK in ObjExpireTime is a mistake but let's keep it as is for now.

[abhijat]

The DCHECK wasn't triggered earlier as we had no asserts which go through the code path where KEEPTTL is passed but object has no expiry, so the object always had a valid expiry, the new assert added at the end of test now does check this code path.

Earlier what happened during test failure was:

1. add object with ttl=100, expiry calculated to 103 (because now() == 3)
2. add object again with keepttl, which ended up setting expiry to 103 in error.
3. check failed because 100 != FIELDTTL

[kostasrim]

Now the math adds up. 100 second ttl, advance by 1 second. afield expired and kttlfield now requires 99 more seconds.