-
Notifications
You must be signed in to change notification settings - Fork 876
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
fix: memcached VERSION is now parseable by php-memcached client #2220
Conversation
src/server/main_service.cc
Outdated
@@ -1341,7 +1341,7 @@ void Service::DispatchMC(const MemcacheParser::Command& cmd, std::string_view va | |||
server_family_.StatsMC(cmd.key, cntx); | |||
return; | |||
case MemcacheParser::VERSION: | |||
mc_builder->SendSimpleString(StrCat("VERSION ", kGitTag)); | |||
mc_builder->SendSimpleString(StrCat("VERSION 1.5.0 DF")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for StrCat()
anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the issue that Dragonfly uses multi-digit numbers (like 1.12.0), whereas memcached expects a single digit for each part?
nope, it uses |
The DF version is being unparseable by Memcached::getVersion() that expects n.n.n string. Change the version to emulate the old memcached server. The DF version can still be fetched via Memcached::getStats() function. Signed-off-by: Roman Gershman <roman@dragonflydb.io>
def test_version(memcached_connection: pymemcache.Client): | ||
""" | ||
php-memcached client expects version to be in the format of "n.n.n", | ||
so we return 1.5.0 emulating an old memcached server. Our real version is being returned in the stats command. Also verified manually that php client parses correctly the version string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern here is that are you sure that this mocked version 1.5.0
is not used internally by the php client?
What if the php client, uses this version to make some internal decision of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the code and I have not seen anything of that kind. Also I succeeded to run the client.
The DF version is being unparseable by Memcached::getVersion() that expects n.n.n string. Change the version to emulate the old memcached server. The DF version can still be fetched via Memcached::getStats() function.