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/09/13 07:57:43 UTC
[GitHub] [incubator-kvrocks] IoCing opened a new pull request, #871: add quit_test
IoCing opened a new pull request, #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871
I find that go-redis don't support quit command.
so i use tcpclient to send command and read result.
--
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 pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245643818
> > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > Just an idea, I am ok with current way : )
>
> @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
>
> ```
> rdb := srv.NewClient().Pipeline()
> result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> rdb.Exec(ctx)
> t.Log(result.Val())
> ```
I think you can just write `rdb.Do(ctx, "quit")` instead of these `formatCommand` stuff.
--
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] IoCing commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245593978
@PragmaTwice thanks for your comments
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r970189466
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
Review Comment:
> Hi, the first `rdb := srv.NewTCPClient()` is not used, I think u can move it into every `t.Run` .Thanks for your great contribution.
I think that the reconnect is not reflected after these changes.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969744652
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
Review Comment:
```suggestion
str := fmt.Sprintf("*%d\r\n", len(args))
for _, arg := range args {
str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
}
```
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245638513
> I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
>
> Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
>
> Just an idea, I am ok with current way : )
@PragmaTwice I'm try this way ,bug get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
```
rdb := srv.NewClient().Pipeline()
result := rdb.Do(ctx, formatCommand([]string{"quit"}))
rdb.Exec(ctx)
t.Log(result.Val())
```
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973591193
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
ok，i revise 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] tanruixiang commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969387552
##########
src/lock_manager.h:
##########
@@ -25,6 +25,7 @@
#include <string>
#include <functional>
+
Review Comment:
Hi. Is this an empty line added accidentally? It seems that it has nothing to do with your pr.
```suggestion
```
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245673433
> > @PragmaTwice It will panic because of EOF. We do get results for this case if we ignore panic.
> > ```
> > pipe := srv.NewClient().Pipeline()
> > tmp1 := pipe.Do(ctx, "quit")
> > tmp2 := pipe.Do(ctx, "ping")
> > _, err := pipe.Exec(ctx)
> > if err != nil { // eof error
> > panic(err)
> > }
> > t.Log(tmp1.Val()) // OK
> > t.Log(tmp2.Val()) // nil
> > ```
>
> A little confused. I think the `panic` is added by yourself rather than in `pipe.Exec`?
[Example code](https://redis.uptrace.dev/guide/go-redis-pipelines.html#pipelines) shows we should check this err.I do not know if we sholud ignore it.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969757313
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
Review Comment:
I think in the original TCL code, `reconnect` is a function. Is there any reason for this inconsistency?
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969815656
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"set", "foo", "bar"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resSet, errSet := rdb.ReadLine()
+ require.Equal(t, "", resSet)
+ require.Error(t, errSet)
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"get", "foo"})))
+ resGet, errGet := rdb.ReadLine()
+ require.Equal(t, "$-1", resGet)
+ require.NoError(t, errGet)
Review Comment:
I think, if it is possible, we would be better when we use the wrapped client instead of raw TCP client.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973587870
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
I think there is no pipelined command in this test?
Here is the corresponding TCL code:
```
test "QUIT returns OK" {
reconnect
assert_equal OK [r quit]
assert_error * {r 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 pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245537513
Thanks for your contribution!
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969759074
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
Review Comment:
In the original TCL code, I think it is not a raw TCP client here. Is there any reason for this inconsistency?
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969757313
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
Review Comment:
I think in the original TCL code, `reconnect` is a function.
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245593347
@PragmaTwice I guess maybe the go-redis not supports quit `Redis 3 commands except QUIT, MONITOR, and SYNC.`.As @IoCing said on the pr.
--
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 merged pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969772496
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"set", "foo", "bar"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resSet, errSet := rdb.ReadLine()
+ require.Equal(t, "", resSet)
+ require.Error(t, errSet)
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"get", "foo"})))
+ resGet, errGet := rdb.ReadLine()
+ require.Equal(t, "$-1", resGet)
+ require.NoError(t, errGet)
Review Comment:
Same as above.
--
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] IoCing commented on pull request #871: add quit_test
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245041571
refer to #857
--
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] IoCing commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1246242095
> @IoCing Hi,I just try `QUIT returns OK`, If we igrone the err, It can run as expected. You can try others.
ok,thanks for your help
--
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 pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245669265
> @PragmaTwice It will panic because of EOF. We do get results for this case if we ignore panic.
>
> ```
> pipe := srv.NewClient().Pipeline()
> tmp1 := pipe.Do(ctx, "quit")
> tmp2 := pipe.Do(ctx, "ping")
> _, err := pipe.Exec(ctx)
> if err != nil { // eof error
> panic(err)
> }
> t.Log(tmp1.Val()) // OK
> t.Log(tmp2.Val()) // nil
> ```
A little confused. I think the `panic` is added by yourself rather than in `rdb.Exec`?
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245661799
@PragmaTwice It will panic because of EOF. We do get results for this case if we ignore panic.
```
pipe := srv.NewClient().Pipeline()
tmp1 := pipe.Do(ctx, "quit")
tmp2 := pipe.Do(ctx, "ping")
_, err := pipe.Exec(ctx)
if err != nil { // eof error
panic(err)
}
t.Log(tmp1.Val()) // OK
t.Log(tmp2.Val()) // nil
```
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245610602
Hi, the first `rdb := srv.NewTCPClient()` is not used, I think u can move it into every `t.Run` .Thanks for your great contribution.
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969791025
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
Review Comment:
In the original TCL code , `reconnect` function is close old client and init a new client with old client's host, port and config. I directly implemented this function here .
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
Review Comment:
because the go-redis do not support quit command in pipelined. I think the same effect can be achieved with tcpclient's `write` and `read`.
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"set", "foo", "bar"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resSet, errSet := rdb.ReadLine()
+ require.Equal(t, "", resSet)
+ require.Error(t, errSet)
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"get", "foo"})))
+ resGet, errGet := rdb.ReadLine()
+ require.Equal(t, "$-1", resGet)
+ require.NoError(t, errGet)
Review Comment:
If I don't understand wrong , In the original TCL code , These two test cases are also very similar
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969823484
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
Review Comment:
Got it. Thanks.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973587870
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
I think we do not need to use pipeline in this test?
Here is the corresponding TCL code:
```
test "QUIT returns OK" {
reconnect
assert_equal OK [r quit]
assert_error * {r 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] PragmaTwice commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973590768
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
If you really want to use pipeline here, I think the identical code will be:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
_, err := pipe.Exec(ctx)
// process cmd1
pipe = rdb.Pipeline()
cmd2 := pipe.Do(ctx, "ping")
_, err = pipe.Exec(ctx)
```
But in your code:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
cmd2 := pipe.Do(ctx, "ping")
_, err := pipe.Exec(ctx)
```
these two command is packed into one pipeline, which is not identical to the TCL code.
--
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] IoCing commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1246087733
> > > @PragmaTwice It will panic because of EOF. We do get results for this case if we ignore panic.
> > > ```
> > > pipe := srv.NewClient().Pipeline()
> > > tmp1 := pipe.Do(ctx, "quit")
> > > tmp2 := pipe.Do(ctx, "ping")
> > > _, err := pipe.Exec(ctx)
> > > if err != nil { // eof error
> > > panic(err)
> > > }
> > > t.Log(tmp1.Val()) // OK
> > > t.Log(tmp2.Val()) // nil
> > > ```
> >
> >
> > A little confused. I think the `panic` is added by yourself rather than in `pipe.Exec`?
>
> [Example code](https://redis.uptrace.dev/guide/go-redis-pipelines.html#pipelines) shows we should check this err. I do not know if we sholud ignore it.
@PragmaTwice @tanruixiang Should I now use `pipeline.do` instead of `tcpclient` reimplementation ?
--
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] IoCing commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1246241752
> > @PragmaTwice @tanruixiang Should I now use `pipeline.do` instead of `tcpclient` reimplementation ?
>
> I prefer `pipeline.do`, although I think both of them are OK to me.
ok , i will try to use pipeline.Do to implement
--
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] tanruixiang commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969797279
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
Review Comment:
```suggestion
package quit
```
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
Review Comment:
```suggestion
rdb := srv.NewTCPClient()
defer func() { require.NoError(t, rdb.Close()) }()
```
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
Review Comment:
```suggestion
rdb := srv.NewTCPClient()
defer func() { require.NoError(t, rdb.Close()) }()
```
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969757313
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
Review Comment:
I think in the original TCL code, `reconnect` is a function, not a comment. Is there any reason for this inconsistency?
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969815656
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"set", "foo", "bar"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resSet, errSet := rdb.ReadLine()
+ require.Equal(t, "", resSet)
+ require.Error(t, errSet)
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"get", "foo"})))
+ resGet, errGet := rdb.ReadLine()
+ require.Equal(t, "$-1", resGet)
+ require.NoError(t, errGet)
Review Comment:
I think, if it is possible, we can use the wrapped client instead of raw TCP client.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969810689
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ suffix := "\r\n"
+ str := "*" + fmt.Sprintf("%d", len(args)) + suffix
+ for _, arg := range args {
+ str += "$" + fmt.Sprintf("%d", len(arg)) + suffix
+ str += arg + suffix
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
Review Comment:
Oh I got it. Thanks.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973590768
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
If you really want to use pipeline here, I think the identical code will be:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
_, err := pipe.Exec(ctx)
// process cmd1
pipe = rdb.Pipeline()
cmd2 := pipe.Do(ctx, "ping")
_, err = pipe.Exec(ctx)
```
But in your code:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
cmd2 := pipe.Do(ctx, "ping")
_, err := pipe.Exec(ctx)
```
these two command is packed into one pipeline, which is not identical to the TCL code.
I think in the original code, the behavior here is request->reply->request->reply, not packed request -> reply.
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
If you really want to use pipeline here, I think the identical code will be:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
_, err := pipe.Exec(ctx)
// process cmd1
pipe = rdb.Pipeline()
cmd2 := pipe.Do(ctx, "ping")
_, err = pipe.Exec(ctx)
```
But in your code:
```
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
cmd2 := pipe.Do(ctx, "ping")
_, err := pipe.Exec(ctx)
```
these two command is packed into one pipeline, which is not identical to the TCL code.
I think in the original code, the behavior here is request->reply->request->reply, not (packed request)->reply.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973590547
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
@IoCing Can see this example: https://github.com/apache/incubator-kvrocks/blob/unstable/tests/gocase/unit/keyspace/keyspace_test.go#L106
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973590323
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
```go
cmd1 := rdb.Do(ctx, "quit")
cmd2 := rdb.Ping(ctx)
require.Equal(t, "OK", cmd1.Val())
require.Equal(t, nil, cmd2.Val())
```
because go-redis don't support `quit`.And i find use `rdb.Do` to excute `quit` at here is also not work.
like above,`ping` should return `nil`, but returns `pong` when actually running.
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r973592686
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package quit
+
+import (
+ "context"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func reconnect(srv *util.KvrocksServer, rdb **redis.Client) error {
+ if err := (*rdb).Close(); err != nil {
+ return err
+ }
+ *rdb = srv.NewClient()
+ return nil
+}
+
+func TestPipeQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewClient()
+ ctx := context.Background()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ require.NoError(t, reconnect(srv, &rdb))
+ pipe := rdb.Pipeline()
+ cmd1 := pipe.Do(ctx, "quit")
+ cmd2 := pipe.Do(ctx, "ping")
+ _, err := pipe.Exec(ctx)
+ require.Equal(t, "OK", cmd1.Val())
+ require.Equal(t, nil, cmd2.Val())
+ require.Error(t, err)
+ })
Review Comment:
```go
require.NoError(t, reconnect(srv, &rdb))
pipe := rdb.Pipeline()
cmd1 := pipe.Do(ctx, "quit")
_, err := pipe.Exec(ctx)
require.Equal(t, "OK", cmd1.Val())
require.NoError(t, err)
// process cmd1
pipe = rdb.Pipeline()
cmd2 := pipe.Do(ctx, "ping")
_, err = pipe.Exec(ctx)
require.Equal(t, nil, cmd2.Val())
require.Error(t, err)
```
This method is not work.
i think i can use tcpclient like what I first implemented.
```go
trdb := srv.NewTCPClient()
require.NoError(t, trdb.Write("*1\r\n$4\r\nquit\r\n"))
resQuit, errQuit := trdb.ReadLine()
require.Equal(t, "+OK", resQuit)
require.NoError(t, errQuit)
require.NoError(t, trdb.Write("*1\r\n$4\r\nping\r\n"))
resPing, errPing := trdb.ReadLine()
require.Equal(t, "", resPing)
require.Error(t, errPing)
```
--
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] IoCing commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969416962
##########
src/lock_manager.h:
##########
@@ -25,6 +25,7 @@
#include <string>
#include <functional>
+
Review Comment:
sorry，i don't know .....
maybe because i run check locally
--
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] tanruixiang commented on a diff in pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969803590
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
Review Comment:
```suggestion
```
I think u can move it into every t.Run.
--
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 #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#discussion_r969815656
##########
tests/gocase/unit/quit_test.go:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package unit
+
+import (
+ "fmt"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/stretchr/testify/require"
+)
+
+func formatCommand(args []string) string {
+ str := fmt.Sprintf("*%d\r\n", len(args))
+ for _, arg := range args {
+ str += fmt.Sprintf("$%d\r\n%s\r\n", len(arg), arg)
+ }
+ return str
+}
+
+func TestQuit(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+ rdb := srv.NewTCPClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("QUIT returns OK", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"ping"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resPing, errPing := rdb.ReadLine()
+ require.Equal(t, "", resPing)
+ require.Error(t, errPing)
+ })
+
+ t.Run("Pipelined commands after QUIT must not be executed", func(t *testing.T) {
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"quit"})))
+ require.NoError(t, rdb.Write(formatCommand([]string{"set", "foo", "bar"})))
+ resQuit, errQuit := rdb.ReadLine()
+ require.Equal(t, "+OK", resQuit)
+ require.NoError(t, errQuit)
+ resSet, errSet := rdb.ReadLine()
+ require.Equal(t, "", resSet)
+ require.Error(t, errSet)
+ // reconnect
+ require.NoError(t, rdb.Close())
+ rdb = srv.NewTCPClient()
+ require.NoError(t, rdb.Write(formatCommand([]string{"get", "foo"})))
+ resGet, errGet := rdb.ReadLine()
+ require.Equal(t, "$-1", resGet)
+ require.NoError(t, errGet)
Review Comment:
I think, if it is possible, we would be better when we use the wrapped client instead of raw TCP client : )
--
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 pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245616479
I am thinking could we use the pipeline support in go-redis instead of current way?
Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
--
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 pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1246200635
> @PragmaTwice @tanruixiang Should I now use `pipeline.do` instead of `tcpclient` reimplementation ?
I prefer `pipeline.do`, although I think both of them are OK.
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1246218520
@IoCing Hi,I just try `QUIT returns OK`, If we igrone the err, It can run as expected. You can try others.
--
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] tanruixiang commented on pull request #871: Move TCL test unit/quit to Go case
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #871:
URL: https://github.com/apache/incubator-kvrocks/pull/871#issuecomment-1245648835
> > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > Just an idea, I am ok with current way : )
>
> @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
>
> ```
> rdb := srv.NewClient().Pipeline()
> result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> rdb.Exec(ctx)
> t.Log(result.Val())
> ```
> > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > Just an idea, I am ok with current way : )
>
> @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
>
> ```
> rdb := srv.NewClient().Pipeline()
> result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> rdb.Exec(ctx)
> t.Log(result.Val())
> ```
> > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > Just an idea, I am ok with current way : )
>
> @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
>
> ```
> rdb := srv.NewClient().Pipeline()
> result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> rdb.Exec(ctx)
> t.Log(result.Val())
> ```
> > > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > > Just an idea, I am ok with current way : )
> >
> >
> > @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
> > ```
> > rdb := srv.NewClient().Pipeline()
> > result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> > rdb.Exec(ctx)
> > t.Log(result.Val())
> > ```
>
> I think you can just write `rdb.Do(ctx, "quit")` instead of these `formatCommand` stuff.
> > > I am wondering whether we could use the pipeline support in go-redis instead of current way. Any ideas?
> > > Refer to https://github.com/go-redis/redis/blob/master/pipeline.go
> > > Just an idea, I am ok with current way : )
> >
> >
> > @PragmaTwice I'm try this way ,but get `unit/pool.go:348: Conn has unread data`,I guess the pipeline still not supports quit?
> > ```
> > rdb := srv.NewClient().Pipeline()
> > result := rdb.Do(ctx, formatCommand([]string{"quit"}))
> > rdb.Exec(ctx)
> > t.Log(result.Val())
> > ```
>
> I think you can just write `rdb.Do(ctx, "quit")` instead of these `formatCommand` stuff.
Oh,sorry ,I did a stupid thing by copy-pasting, I'll try it again.
--
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