You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/12/04 09:51:08 UTC
[GitHub] [incubator-kvrocks] xiaobiaozhao opened a new pull request, #1158: feat: Fit redis driver
xiaobiaozhao opened a new pull request, #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158
add redis_version
add kvrocks_version
close https://github.com/apache/incubator-kvrocks/issues/1157
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#issuecomment-1339662893
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1041913044
##########
src/server/server.cc:
##########
@@ -44,6 +44,7 @@
#include "worker.h"
std::atomic<int> Server::unix_time_ = {0};
+constexpr const char *REDIS_VERSION = "4.0.0";
Review Comment:
Yes, we are roughly aligning the protocol with the Redis 4.x, but the data type and command can be newer than 4.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038984111
##########
src/server/server.cc:
##########
@@ -783,6 +783,7 @@ void Server::GetServerInfo(std::string *info) {
time(&now);
string_stream << "# Server\r\n";
string_stream << "version:" << VERSION << "\r\n";
Review Comment:
```suggestion
string_stream << "kvrocks_version:" << VERSION << "\r\n";
```
I am wondering if it will be better since client frameworks can determine whether the server is kvrocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038974133
##########
src/version.h.in:
##########
@@ -21,4 +21,5 @@
#pragma once
#define VERSION "@PROJECT_VERSION@"
+#define REDIS_VERSION "4.0.0"
Review Comment:
I think it should not be in `version.h.in`, which is a header for kvrocks version configuration (not redis, or other stuff).
You can put them just in `server.h`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1041768278
##########
src/server/server.cc:
##########
@@ -44,6 +44,7 @@
#include "worker.h"
std::atomic<int> Server::unix_time_ = {0};
+constexpr const char *REDIS_VERSION = "4.0.0";
Review Comment:
Out of curiosity, does it mean that we're functionality compatible with Redis 4.x?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038989997
##########
src/server/server.cc:
##########
@@ -783,6 +783,7 @@ void Server::GetServerInfo(std::string *info) {
time(&now);
string_stream << "# Server\r\n";
string_stream << "version:" << VERSION << "\r\n";
Review Comment:
This may break the old behavior like Kvrock exporter. I prefer to keep the `version`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1041769036
##########
src/server/server.cc:
##########
@@ -44,6 +44,7 @@
#include "worker.h"
std::atomic<int> Server::unix_time_ = {0};
+constexpr const char *REDIS_VERSION = "4.0.0";
Review Comment:
But we support STREAM delivered in Redis 5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#issuecomment-1339239641
@PragmaTwice @ShooterIT Does it look good to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1041932878
##########
src/server/server.cc:
##########
@@ -44,6 +44,7 @@
#include "worker.h"
std::atomic<int> Server::unix_time_ = {0};
+constexpr const char *REDIS_VERSION = "4.0.0";
Review Comment:
I also think in the future there will be some commands that only kvrocks has but redis doesn't
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1158: feat: Fit redis driver
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038967855
##########
src/server/server.cc:
##########
@@ -783,6 +783,8 @@ void Server::GetServerInfo(std::string *info) {
time(&now);
string_stream << "# Server\r\n";
string_stream << "version:" << VERSION << "\r\n";
+ string_stream << "redis_version:" << REDIS_VERSION << "\r\n";
+ string_stream << "kvrocks_version:" << VERSION << "\r\n";
Review Comment:
Need to add `REDIS_VERSION` first and remove the duplicate `kvrocks_version`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038974133
##########
src/version.h.in:
##########
@@ -21,4 +21,5 @@
#pragma once
#define VERSION "@PROJECT_VERSION@"
+#define REDIS_VERSION "4.0.0"
Review Comment:
I think it should not be in `version.h.in`, which is a header for kvrocks version configuration.
You can put them just in `server.h`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038974133
##########
src/version.h.in:
##########
@@ -21,4 +21,5 @@
#pragma once
#define VERSION "@PROJECT_VERSION@"
+#define REDIS_VERSION "4.0.0"
Review Comment:
I think it should not be in `version.h.in`, which is a header for kvrocks version configuration (not redis, or other stuff).
You can put them just in `server.h/.cc`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038989510
##########
src/server/server.cc:
##########
@@ -783,6 +783,7 @@ void Server::GetServerInfo(std::string *info) {
time(&now);
string_stream << "# Server\r\n";
string_stream << "version:" << VERSION << "\r\n";
Review Comment:
@git-hulk how about you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038984111
##########
src/server/server.cc:
##########
@@ -783,6 +783,7 @@ void Server::GetServerInfo(std::string *info) {
time(&now);
string_stream << "# Server\r\n";
string_stream << "version:" << VERSION << "\r\n";
Review Comment:
```suggestion
string_stream << "kvrocks_version:" << VERSION << "\r\n";
```
I am wondering if it will be better since client frameworks can determine the server is kvrocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1158: Add the compatible Redis version to the info command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1158:
URL: https://github.com/apache/incubator-kvrocks/pull/1158#discussion_r1038990066
##########
src/version.h.in:
##########
@@ -21,4 +21,5 @@
#pragma once
#define VERSION "@PROJECT_VERSION@"
+#define REDIS_VERSION "4.0.0"
Review Comment:
Yes, twice is right here, I didn't notice this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org