[WIP] (string_family): Add support for MC GAT #5199
Conversation
abhijat
force-pushed
the
abhijat/feat/add-mc-gat-support
branch
6 times, most recently
from
961e1fa to
22462ee
Compare
abhijat
force-pushed
the
abhijat/feat/add-mc-gat-support
branch
6 times, most recently
from
68f7a06 to
d346ac3
Compare
The memcache protocol commands GAT and GATS should use the same code path as MC GET, but also touch expiry of keys which are searched. The expiry is stored in memcache flag of connection context. The mget code path is changed to expire keys if a combination of mc protocol and the set_expiry flag are found. Signed-off-by: Abhijat Malviya <[email protected]>
abhijat
force-pushed
the
abhijat/feat/add-mc-gat-support
branch
from
d346ac3 to
e26645e
Compare
|
I suggest that you split this PR into a "trivial" part that handles, parses the GAT command, parses the ttl but ignores the "touch" part and basically still calls MGET as is. The second part - of actually touching should be a separate PR and we should talk about it. I did not estimate correctly the complexity involved with this part. I would argue it's not MGET - it's more LIKE "SET with GET" option - because it's writeable https://www.dragonflydb.io/docs/command-reference/strings/set. So let's merge the trivial part first. |
| MGetResponse OpMGet(BlockingCounter wait_bc, const Transaction* t, EngineShard* shard, | ||
| const MemcacheMGetParams& mc_params) { | ||
| const ShardArgs keys = t->GetShardArgs(shard->shard_id()); | ||
| DCHECK(!keys.Empty()); | ||
|
|
||
| auto& db_slice = t->GetDbSlice(shard->shard_id()); | ||
|
|
||
| MGetResponse response(keys.Size()); | ||
|
|
||
| auto [items, key_index, total_size] = | ||
| FindAndMaybeExpireKeys(t, shard, keys, db_slice, mc_params.expire_params()); |
There was a problem hiding this comment.
MGET is readonly, we can't just casually delete a few values in case someone sets a negative (i.e past) expire time 🤔
There was a problem hiding this comment.
I see, thanks. I added a separate command id for GAT which has a CO::Write flag, it reuses the MGet method, but doesn't invoke the same command as MGET, but it looks like it is not safe to reuse the method?
There was a problem hiding this comment.
Ah, didn't notice that you added it. If so, it is, but I'd suggest to document this place properly and maybe move the conditional to the top, so we can see it's not invoced for the usual code path
created #5229 FWIW it looks like much of the code will remain similar for GAT as MGET, but the iterator type for the search and the command flag is different. If a new command has to be added, it might make sense to reuse code from the MGET code path. |
Support for GAT command is added in this change set.
The memcache protocol commands GAT and GATS will use the same code path as MC GET/GETS, and also touch expiry of keys which are found using the supplied expiry.
The MGET code path is changed to optionally set expiry of keys it finds.