You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "enjoy-binbin (via GitHub)" <gi...@apache.org> on 2023/04/18 14:46:21 UTC
[GitHub] [incubator-kvrocks] enjoy-binbin opened a new pull request, #1394: Add TIME command as Redis
enjoy-binbin opened a new pull request, #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394
This PR adds the TIME command. The TIME command returns the current
server time as a two items lists: a Unix timestamp and the amount of
microseconds already elapsed in the current second.
Example:
```
127.0.0.1:6666> time
1) "1681828105"
2) "810037"
```
This closes #1393
--
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] enjoy-binbin commented on pull request #1394: Add TIME command as Redis
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#issuecomment-1513281057
having trouble with how to add a time test case...
--
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] enjoy-binbin commented on a diff in pull request #1394: Add TIME command as Redis
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170217843
##########
src/commands/cmd_server.cc:
##########
@@ -941,6 +957,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandAuth>("auth", 2, "read-only ok-loadin
MakeCmdAttr<CommandDebug>("debug", -2, "read-only exclusive", 0, 0, 0),
MakeCmdAttr<CommandCommand>("command", -1, "read-only", 0, 0, 0),
MakeCmdAttr<CommandEcho>("echo", 2, "read-only", 0, 0, 0),
+ MakeCmdAttr<CommandTime>("time", 1, "ok-loading", 0, 0, 0),
Review Comment:
updated, thanks for the catch
--
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 #1394: Add TIME command as Redis
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#issuecomment-1514212974
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] torwig commented on a diff in pull request #1394: Add TIME command as Redis
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170185753
##########
src/commands/cmd_server.cc:
##########
@@ -941,6 +957,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandAuth>("auth", 2, "read-only ok-loadin
MakeCmdAttr<CommandDebug>("debug", -2, "read-only exclusive", 0, 0, 0),
MakeCmdAttr<CommandCommand>("command", -1, "read-only", 0, 0, 0),
MakeCmdAttr<CommandEcho>("echo", 2, "read-only", 0, 0, 0),
+ MakeCmdAttr<CommandTime>("time", 1, "ok-loading", 0, 0, 0),
Review Comment:
I think you can add `read-only` to the attributes of the `TIME` command.
--
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] torwig commented on a diff in pull request #1394: Add TIME command as Redis
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170234533
##########
tests/gocase/unit/introspection/introspection_test.go:
##########
@@ -40,6 +40,24 @@ func TestIntrospection(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
+ t.Run("TIME", func(t *testing.T) {
+ now_ms := int(time.Now().Unix())
Review Comment:
@enjoy-binbin Just a little fix - in Go, the snakeCase is preferred, so you can rename the variable to `now`/`nowMs`.
P.S. According to the docs, `time.Now().Unix()` returns seconds not milliseconds. `UnixMilli()` returns milliseconds if you need milliseconds. Otherwise, the variable that holds seconds shouldn't hold seconds.
--
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] enjoy-binbin commented on pull request #1394: Add TIME command as Redis
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#issuecomment-1513370138
@git-hulk thanks. i got it now
--
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 #1394: Add TIME command as Redis
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#issuecomment-1513299484
> having trouble with how to add a time test case...
@enjoy-binbin Do you mean that you don't know how to check if the time command is correct? If yes, we can only check if the seconds field is in the range of [now-2, now+2] by using `require.WithinRange`.
And if you don't know where to add the test case, can just imitate the [ping command test case](https://github.com/apache/incubator-kvrocks/tree/unstable/tests/gocase/unit/ping).
--
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 #1394: Add TIME command as Redis
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170756980
##########
tests/gocase/unit/introspection/introspection_test.go:
##########
@@ -40,6 +40,24 @@ func TestIntrospection(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
+ t.Run("TIME", func(t *testing.T) {
+ nowUnix := int(time.Now().Unix())
+
+ r := rdb.Do(ctx, "TIME")
+ vs, err := r.Slice()
+ require.NoError(t, err)
+
+ s, err := strconv.Atoi(vs[0].(string))
+ require.NoError(t, err)
+ require.Greater(t, s, nowUnix - 2)
+ require.Less(t, s, nowUnix + 2)
Review Comment:
No worry about the Go fmt, we can auto-fix it in the future.
```suggestion
require.Greater(t, s, nowUnix-2)
require.Less(t, s, nowUnix+2)
```
--
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 #1394: Add TIME command as Redis
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170737484
##########
src/commands/cmd_server.cc:
##########
@@ -641,6 +642,21 @@ class CommandEcho : public Commander {
}
};
+class CommandTime : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ uint64_t now = util::GetTimeStampUS();
+ uint64_t s = now / 1000 / 1000; // unix time in seconds.
Review Comment:
```suggestion
uint64_t s = now / 1000 / 1000; // unix time in seconds.
```
--
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] enjoy-binbin commented on a diff in pull request #1394: Add TIME command as Redis
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on code in PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394#discussion_r1170253563
##########
tests/gocase/unit/introspection/introspection_test.go:
##########
@@ -40,6 +40,24 @@ func TestIntrospection(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
+ t.Run("TIME", func(t *testing.T) {
+ now_ms := int(time.Now().Unix())
Review Comment:
oh right, bad habit... thanks a lot.
--
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 #1394: Add TIME command as Redis
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1394:
URL: https://github.com/apache/incubator-kvrocks/pull/1394
--
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