Skip to content

Implementation of memcache GAT and GATS commands #5257

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Implementation of memcache GAT and GATS commands #5257

wants to merge 6 commits into from

Conversation

abhijat
Copy link
Contributor

@abhijat abhijat commented Jun 9, 2025 •
edited
Loading

A new command is added to the string family called GAT which implements the memcache get and touch semantics. The command syntax is GAT <expiry-seconds> key [keys...].

Most of the changes in the PR are related to extracting common code from the MGET path to be reused for GAT.

A collection function is extracted which takes a search function to search for an individual key and collect results across many keys. Search has a dbslice iterator template parameter which can be mutable or immutable, depending on the command invoking it.

A runtime check is made such that a read only command does not invoke the helper with a mutable iterator.

Follow up of #5199

The GATS command is also implemented, it returns a placeholder CAS version of 0 for now.

fixes #5089

@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch 11 times, most recently from f68ee24 to 744e0a6 Compare June 10, 2025 08:10
@abhijat abhijat changed the title WIP (do not review): GAT part 2 WIP: implementation of memcache GAT command Jun 10, 2025
@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch 8 times, most recently from 6c94075 to 54d0eb0 Compare June 10, 2025 13:47
abhijat
abhijat commented Jun 10, 2025
View reviewed changes
src/server/main_service.cc Outdated
Comment on lines 1645 to 1649
if (cmd.type == MemcacheParser::GAT) {
char* next = absl::numbers_internal::FastIntToBuffer(expire_ts, ttl);
args.emplace_back(ttl, next - ttl);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

expire_ts and its source: cmd.expire_ts are unsigned values. The memcache protocol specifies that a negative expiry is used to immediately expire a key, perhaps we should use signed values here to support that kind of expiry.

abhijat
abhijat commented Jun 11, 2025
View reviewed changes
src/server/conn_context.h
FETCH_CAS_VER = 1,
SET_EXPIRY = 2,
};
enum MCGetMask { FETCH_CAS_VER = 1 };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes (within this file) are reverted from the previous PR. Because we now have a separate command for GAT, we no longer need to stash the expiry in the memcache flag, it is passed explicitly as part of the command.

@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch 2 times, most recently from 9eae7df to da62b7d Compare June 11, 2025 06:17
@abhijat abhijat marked this pull request as ready for review June 11, 2025 07:02
@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch from da62b7d to f83e819 Compare June 11, 2025 07:04
@abhijat abhijat changed the title WIP: implementation of memcache GAT command Implementation of memcache GAT command Jun 11, 2025
@abhijat

This comment was marked as resolved.

Sign in to view
@abhijat

This comment was marked as resolved.

Sign in to view
@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch from 7183acc to 9bc5cbc Compare June 12, 2025 05:38
@abhijat abhijat changed the title Implementation of memcache GAT command Implementation of memcache GAT and GATS commands Jun 12, 2025
@abhijat
Copy link
Contributor Author

abhijat commented Jun 12, 2025

GATS is not implemented in this PR, but if we have to return the placeholder value for cas version similar to GETS , then the same command GAT can handle GATS as well making use of the memcache flag.

As discussed elsewhere we should still return a placeholder, but I would prefer to do a follow up PR, mainly to add tests specific to GATS.

The actual code additions required for GATS are very small.

Added implementation for GATS, the tests are done through the same pytest code which tests GAT

romange
romange reviewed Jun 12, 2025
View reviewed changes
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

it's quite a big PR. given that you needed to do some refactoring in string family, can
you please split into a refactoring PR that does not change the functionality first?

src/server/main_service.cc Outdated
@@ -1642,17 +1645,18 @@ void Service::DispatchMC(const MemcacheParser::Command& cmd, std::string_view va
}
dfly_cntx->conn_state.memcache_flag = cmd.flags;
} else if (cmd.type < MemcacheParser::QUIT) { // read commands
if (cmd.type == MemcacheParser::GAT || cmd.type == MemcacheParser::GATS) {
char* next = absl::numbers_internal::FastIntToBuffer(expire_ts, ttl);
args.insert(args.begin() + 1, {ttl, static_cast<size_t>(next - ttl)});
Copy link
Collaborator

Choose a reason for hiding this comment

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

add here a comment demonstrating how a command looks like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reordered the code a bit so we can emplace back instead of inserting in the middle, also added a comment.

abhijat added 3 commits June 13, 2025 09:34
@abhijat
chore(string_family): Refactor common code from MGet
e5721b8 
A couple of functions are extracted from the MGet command. The reason
for this is that in a follow up PR, the GAT command will be added which
will reuse some of the functions:

A function to collect keys is extracted. This takes a function as input
which will return an iterator for one key. The iterator can be mutable
or immutable, for MGet it is immutable. For GAT it will be mutable.

A check is added such that a read-only command does not use a function
returning a mutable iterator.

Another function is created to reorder collected results based on the
shards of the keys and the original order of the keys in the command.

Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat
chore(string_family): Extract variable for value
39eaf8e 
Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat
chore(memcache): Revert changes to connection ctx
7aed4e9 
The flag for memcache will no longer carry the expiration timestamp.
This is passed as a part of the command. The changes done previously to
support this are reverted as these are no longer necessary.

Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch from 9bc5cbc to f77dcfd Compare June 13, 2025 04:21
@abhijat
Copy link
Contributor Author

abhijat commented Jun 13, 2025

it's quite a big PR. given that you needed to do some refactoring in string family, can you please split into a refactoring PR that does not change the functionality first?

Created PR with the refactor specific changes #5289

@abhijat abhijat mentioned this pull request Jun 13, 2025
Open
abhijat added 3 commits June 13, 2025 16:19
@abhijat
feat(memcache): Add GAT command to support MC GAT, GATS
3b928dd 
A new string family command, GAT, is added to support memcache commands
GAT and GATS. The format of the command is:

GAT|GATS <exp> keys..

The command reuses code extracted from MGet in a previous commit. GAT is
a write command, it changes expiry of keys along with searching for them
and returning found keys.

Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat
feat(memcache): Unit tests for GAT,GATS
52e284e 
Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat
feat(memcache): Integration tests for GAT,GATS
97cdb4b 
Signed-off-by: Abhijat Malviya <[email protected]>
@abhijat abhijat force-pushed the abhijat/feat/gat-cmd branch from f77dcfd to 97cdb4b Compare June 13, 2025 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romange romange romange left review comments

@BorysTheDev BorysTheDev Awaiting requested review from BorysTheDev

@adiholden adiholden Awaiting requested review from adiholden

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support GAT/GATS in memcache
2 participants
@abhijat @romange