You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "git-hulk (via GitHub)" <gi...@apache.org> on 2023/05/22 09:15:40 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #1464: Check if the cluster migrate sync command will get error
git-hulk opened a new pull request, #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464
Currently, the migration timeout test case failed frequently when running in GitHub Actions and the value result is Nil. But we cannot know if it's caused by an error or if the result value is a nil string, so we can need to check if it occurs in error to narrow the scope.
This PR is for reproducing the test case failure.
--
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 #1464: Fix flaky test in migration timeout
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202261074
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
I see that right now timeout < 0 doesn't produce an error. Should we introduce an error in this case? Or just treat it as an absence of a timeou?
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
I see that right now timeout < 0 doesn't produce an error. Should we introduce an error in this case? Or just treat it as an absence of a timeout?
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202287815
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
For the negative timeout, I also prefer retrieving an error instead of regarding it as 0.
> Could we use []float64 here instead of []interface{}?
It's intent to use the interface{} since I wanna make sure that the int and float types are working well.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201537078
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
So when I use a very small time parameter causing a failure, the program calls `resume` func and returns `nil` value. Maybe we need the following test.
```go
// for result.Val() == nil && timeout < 15 {
// timeout += 5
// result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
// require.NoError(t, result.Err())
// }
// require.Equal(t, "OK", 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] git-hulk commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202392585
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
@ellutionist Another concern is float timeout may be inconsistent with other commands.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202540403
##########
src/cluster/sync_migrate_context.h:
##########
@@ -25,7 +25,7 @@
class SyncMigrateContext : private EvbufCallbackBase<SyncMigrateContext, false>,
private EventCallbackBase<SyncMigrateContext> {
public:
- SyncMigrateContext(Server *svr, redis::Connection *conn, float timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
+ SyncMigrateContext(Server *svr, redis::Connection *conn, int timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
Review Comment:
We should do it in util::Parse function instead of here.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202316457
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
I will append a commit to resolve above comments.
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
I will append a commit to resolve the above 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] git-hulk merged pull request #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464
--
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 #1464: Fix flaky test in migration timeout
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202515406
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -435,17 +435,15 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync timeout", func(t *testing.T) {
- cnt := 200000
slot++
+ cnt := 200000
for i := 0; i < cnt; i++ {
require.NoError(t, rdb0.LPush(ctx, util.SlotTable[slot], i).Err())
}
- timeout := 0.001
-
- require.Nil(t, rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout).Val())
- // check the following command on the same connection
- require.Equal(t, "PONG", rdb0.Ping(ctx).Val())
Review Comment:
@git-hulk Yes, absolutely.
--
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 #1464: Fix flaky test in migration timeout
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202258918
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
Could we use `[]float64` here instead of `[]interface{}`?
If the timeout is a negative value, should it be an error?
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201537078
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
So when I use a very small time parameter causing a failure, the program calls `resume` func and returns `nil` value. Maybe we need the following test.
But the following statements may fail from my tests.
```go
// for result.Val() == nil && timeout < 15 {
// timeout += 5
// result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
// require.NoError(t, result.Err())
// }
// require.Equal(t, "OK", result.Val())
```
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
So when I use a very small time parameter causing a failure, the program calls `resume` func and returns `nil` value. Maybe we need the following test. But the following statements may fail from my tests.
```go
// for result.Val() == nil && timeout < 15 {
// timeout += 5
// result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
// require.NoError(t, result.Err())
// }
// require.Equal(t, "OK", 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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202524743
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -418,12 +418,19 @@ func TestSlotMigrateSync(t *testing.T) {
require.NoError(t, result.Err())
require.Equal(t, "OK", result.Val())
- timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
- for _, timeout := range timeouts {
+ invalidTimeouts := []interface{}{-2, -1, "abc"}
Review Comment:
Does it need to add a float argument to show the above dicussion?
##########
src/commands/cmd_cluster.cc:
##########
@@ -146,7 +146,14 @@ class CommandClusterX : public Commander {
sync_migrate_ = true;
if (args.size() == 6) {
- sync_migrate_timeout_ = GET_OR_RET(ParseFloat<float>(args[5]));
+ auto parse_result = ParseInt<int>(args[5], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, "timeout is not an integer or out of range"};
+ }
+ if (*parse_result < 0) {
+ return {Status::RedisParseErr, "timeout should not be negative"};
Review Comment:
```suggestion
return {Status::RedisParseErr, errTimeoutIsNegative};
```
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202540403
##########
src/cluster/sync_migrate_context.h:
##########
@@ -25,7 +25,7 @@
class SyncMigrateContext : private EvbufCallbackBase<SyncMigrateContext, false>,
private EventCallbackBase<SyncMigrateContext> {
public:
- SyncMigrateContext(Server *svr, redis::Connection *conn, float timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
+ SyncMigrateContext(Server *svr, redis::Connection *conn, int timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
Review Comment:
We should do it in util::ParseXXX test cases instead of here.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201446487
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
Sure. To be honest, we didn't take care of those slot distributions when writing test cases, so it's a bit confusing for using which slot 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] ellutionist commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202015912
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -422,7 +422,7 @@ func TestSlotMigrateSync(t *testing.T) {
for _, timeout := range timeouts {
slot++
result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
- require.NoError(t, result.Err())
+ require.NoError(t, result.Err(), "without timeout: %v", timeout)
Review Comment:
Should the message be "with timeout"?
--
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 #1464: Check if the cluster migrate sync command will get error
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#issuecomment-1558375777
We have reproduced this flaky error: https://github.com/apache/incubator-kvrocks/actions/runs/5044005766/jobs/9046835096#step:12:57. It's caused by the slot conflict when running the migration, let's increase the start slot to avoid this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202308754
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
Good catch, I didn't dive into the implementation. For the server side, it allows the float timeout now, but I think it's unnecessary to support the float timeout.
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
Good catch, I didn't dive into the implementation. For the server side, it allows the float timeout now, but I think it's unnecessary to support that.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202544457
##########
src/cluster/sync_migrate_context.h:
##########
@@ -25,7 +25,7 @@
class SyncMigrateContext : private EvbufCallbackBase<SyncMigrateContext, false>,
private EventCallbackBase<SyncMigrateContext> {
public:
- SyncMigrateContext(Server *svr, redis::Connection *conn, float timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
+ SyncMigrateContext(Server *svr, redis::Connection *conn, int timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
Review Comment:
we use `Parsefloat` to parse `geo` positions, hash computing, some command with concrete needs, such as `ZADD`, `INCRBYFLOAT`.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201539520
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
Yes, [8c76f33](https://github.com/apache/incubator-kvrocks/pull/1464/commits/8c76f3339b344b13a9964a075d08737c9e95bb56) this commit has increased the timeout.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201537078
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
So when I use a very small time parameter causing a failure, the program calls `resume` func and returns `nil` value. Maybe we need the following test. But the following statements may fail from my tests with throwing `There is already a migrating slot`.
```go
// for result.Val() == nil && timeout < 15 {
// timeout += 5
// result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
// require.NoError(t, result.Err())
// }
// require.Equal(t, "OK", 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] git-hulk commented on pull request #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#issuecomment-1560274913
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] ellutionist commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202016720
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -422,7 +422,7 @@ func TestSlotMigrateSync(t *testing.T) {
for _, timeout := range timeouts {
slot++
result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
- require.NoError(t, result.Err())
+ require.NoError(t, result.Err(), "without timeout: %v", timeout)
require.Equal(t, "OK", result.Val())
Review Comment:
Perhaps add "with timeout" message here also?
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201530690
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
The reason for this error is that the sync time is too small such as `0.5s` and CI often runs failed while local machines run successful. And the commit https://github.com/apache/incubator-kvrocks/commit/635dd311e5d07007826ae5338eae4cefb88ccc55 also improves read success rate.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202026873
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
+ for _, timeout := range timeouts {
+ slot++
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
+ require.NoError(t, result.Err(), "without timeout: %v", timeout)
+ require.Equal(t, "OK", result.Val())
Review Comment:
```suggestion
require.NoError(t, result.Err(), "with timeout: %v", timeout)
require.Equal(t, "OK", result.Val(), "with timeout: %v", timeout)
```
--
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 #1464: Fix flaky test in migration timeout
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202295318
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
@git-hulk Do we ever need floats there? I see that float is cast to int in the code, so I think we should consider timeout as an integer number of seconds and prohibit float and negative values.
--
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 #1464: Fix flaky test in migration timeout
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202336424
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
@ellutionist Timeout is in seconds as far as I see so I don't think that making the timeout less than 1 second is reasonable (it's about migrating slot with data from one node to another via network).
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202024812
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -422,7 +422,7 @@ func TestSlotMigrateSync(t *testing.T) {
for _, timeout := range timeouts {
slot++
result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
- require.NoError(t, result.Err())
+ require.NoError(t, result.Err(), "without timeout: %v", timeout)
Review Comment:
Yes, it's a typo.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202519121
##########
src/commands/cmd_cluster.cc:
##########
@@ -146,7 +146,14 @@ class CommandClusterX : public Commander {
sync_migrate_ = true;
if (args.size() == 6) {
- sync_migrate_timeout_ = GET_OR_RET(ParseFloat<float>(args[5]));
+ auto parse_result = ParseInt<int>(args[5], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, "timeout is not an integer or out of range"};
+ }
+ if (*parse_result < 0) {
+ return {Status::RedisParseErr, "timeout should not be negative"};
Review Comment:
```suggestion
return {Status::RedisParseErr, errTimeoutIsNegative};
```
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#issuecomment-1559633418
@torwig I fixed the sync timeout test case as well in the last commit: https://github.com/apache/incubator-kvrocks/pull/1464/commits/bcc7b8509ab69f432f42c5cdeda43855ea066f4f, please take a look again. After changing the timeout to int, then use the float timeout would return an error.
--
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] ellutionist commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202324820
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
@torwig @git-hulk The original reason to support float timeout is that in the go tests I want to "deliberately let the sync migration timeout" (see [here](https://github.com/apache/incubator-kvrocks/blob/unstable/tests/gocase/integration/slotmigrate/slotmigrate_test.go#L440)). And the minimal integer value of "1" is too large and cannot ensure the timeout happen...
Also I think supporting float timeout is not harmful but provides more precise control to the migration. What do you think?
As for the negative value, returning an error when the server receives an negative value seems acceptable to me. No problem.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202528682
##########
src/cluster/sync_migrate_context.h:
##########
@@ -25,7 +25,7 @@
class SyncMigrateContext : private EvbufCallbackBase<SyncMigrateContext, false>,
private EventCallbackBase<SyncMigrateContext> {
public:
- SyncMigrateContext(Server *svr, redis::Connection *conn, float timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
+ SyncMigrateContext(Server *svr, redis::Connection *conn, int timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
Review Comment:
BTW, shall we add a float test to gtests with improving code coverage?
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201537078
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
So when I use a very small time parameter causing a failure, the program calls `resume` func and returns `nil` value. Maybe we need the following test. But the following statements may
fail from my test
```go
// for result.Val() == nil && timeout < 15 {
// timeout += 5
// result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout)
// require.NoError(t, result.Err())
// }
// require.Equal(t, "OK", 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] tisonkun commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1201439974
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -412,23 +412,19 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ slot := 1024
Review Comment:
Can we add a comment for this magic number? From the description it's to avoid slot num conflict.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202287815
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,22 +413,18 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -3.14).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ timeouts := []interface{}{-1, 0, 20, 10.5, -3.14}
Review Comment:
For the negative timeout, I also prefer retrieving an error instead of regarding it as 0. But I think we can submit another PR to fix this.
> Could we use []float64 here instead of []interface{}?
It's intent to use the interface{} since I wanna make sure that the int and float types are working well.
--
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] infdahai commented on a diff in pull request #1464: Fix flaky test in migration timeout
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202544457
##########
src/cluster/sync_migrate_context.h:
##########
@@ -25,7 +25,7 @@
class SyncMigrateContext : private EvbufCallbackBase<SyncMigrateContext, false>,
private EventCallbackBase<SyncMigrateContext> {
public:
- SyncMigrateContext(Server *svr, redis::Connection *conn, float timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
+ SyncMigrateContext(Server *svr, redis::Connection *conn, int timeout) : svr_(svr), conn_(conn), timeout_(timeout){};
Review Comment:
we only use `Parsefloat` now in kvrocks to parse `geo` positions, hash computing, some command with concrete needs, such as `ZADD`, `INCRBYFLOAT`.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202512425
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -435,17 +435,15 @@ func TestSlotMigrateSync(t *testing.T) {
})
t.Run("MIGRATE - Migrate sync timeout", func(t *testing.T) {
- cnt := 200000
slot++
+ cnt := 200000
for i := 0; i < cnt; i++ {
require.NoError(t, rdb0.LPush(ctx, util.SlotTable[slot], i).Err())
}
- timeout := 0.001
-
- require.Nil(t, rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", timeout).Val())
- // check the following command on the same connection
- require.Equal(t, "PONG", rdb0.Ping(ctx).Val())
Review Comment:
This line won' take effect since we're using the connection pool.
--
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 #1464: Fix flaky test in migration timeout
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1464:
URL: https://github.com/apache/incubator-kvrocks/pull/1464#discussion_r1202533175
##########
tests/gocase/integration/slotmigrate/slotmigrate_test.go:
##########
@@ -413,36 +413,37 @@ func TestSlotMigrateSync(t *testing.T) {
t.Run("MIGRATE - Migrate sync with (or without) all kinds of timeouts", func(t *testing.T) {
slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync").Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", -1).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 10).Val())
-
- slot++
- require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync", 0.5).Val())
+ // migrate sync without explicitly specify the timeout
+ result := rdb0.Do(ctx, "clusterx", "migrate", slot, id1, "sync")
+ require.NoError(t, result.Err())
+ require.Equal(t, "OK", result.Val())
+
+ invalidTimeouts := []interface{}{-2, -1, "abc"}
Review Comment:
```suggestion
invalidTimeouts := []interface{}{-2, -1, 1.2, "abc"}
```
--
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