Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix flaky test in migration timeout #1464

Merged
merged 10 commits into from
May 24, 2023
Merged

Fix flaky test in migration timeout #1464

merged 10 commits into from
May 24, 2023

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented May 22, 2023 •
edited
Loading

This closes #1452

git-hulk added 3 commits May 22, 2023 17:14
@git-hulk
Check if the cluster migrate sync command will get error
f1935f1 
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 error or the result value is a nil string, so we
can need to check if it occurs error to narrow the scope.
@git-hulk
Merge branch 'unstable' into check-error-in-migration-timeout-test-case
307d6dd
@git-hulk
Fix slot conflicts in migration test case
bdfdc92
@git-hulk
Copy link
Member Author

git-hulk commented May 23, 2023 •
edited
Loading

We have reproduced this flaky error: https://github.com/apache/incubator-kvrocks/actions/runs/5044728692/jobs/9055654883. It's caused by the slot conflict when running the migration, let's increase the start slot to avoid this.

@git-hulk git-hulk marked this pull request as ready for review May 23, 2023 01:51
@git-hulk git-hulk changed the title Check if the cluster migrate sync command will get error Fix flaky test in migration timeout May 23, 2023
infdahai
infdahai previously approved these changes May 23, 2023
View reviewed changes
torwig
torwig previously approved these changes May 23, 2023
View reviewed changes
@git-hulk git-hulk dismissed stale reviews from torwig and infdahai via f800158 May 23, 2023 10:28
torwig
torwig previously approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@git-hulk
Copy link
Member Author

@torwig I fixed the sync timeout test case as well in the last commit: bcc7b85, please take a look again. After changing the timeout to int, then use the float timeout would return an error.

git-hulk
git-hulk commented May 23, 2023
View reviewed changes
tests/gocase/integration/slotmigrate/slotmigrate_test.go

// check the following command on the same connection
require.Equal(t, "PONG", rdb0.Ping(ctx).Val())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line won' take effect since we're using the connection pool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@git-hulk Yes, absolutely.

infdahai
infdahai suggested changes May 23, 2023
View reviewed changes
@git-hulk @infdahai
Update src/commands/cmd_cluster.cc
0688e34 
Co-authored-by: clundro <infdahai@outlook.com>
@git-hulk
Copy link
Member Author

Thanks all, merging...

@git-hulk git-hulk merged commit 4fc4412 into apache:unstable May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infdahai infdahai infdahai requested changes

@torwig torwig torwig approved these changes

@PragmaTwice PragmaTwice Awaiting requested review from PragmaTwice

@tisonkun tisonkun Awaiting requested review from tisonkun

@ellutionist ellutionist Awaiting requested review from ellutionist

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

migrate timeout in Go Integration Cases
5 participants
@git-hulk @torwig @tisonkun @infdahai @ellutionist