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.

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

Tabletmanager tests cases migrated from Python #5630

Merged
merged 2 commits into from
Jan 3, 2020

Conversation

ajeetj
Copy link
Contributor

@ajeetj ajeetj commented Dec 31, 2019

Adding/Fixing test cases that were flaky before.
Signed-off-by: Ajeet jain ajeet@planetscale.com

@ajeetj ajeetj requested a review from sougou as a code owner December 31, 2019 10:12
@ajeetj
Copy link
Contributor Author

ajeetj commented Dec 31, 2019

@deepthi Need your help to debug why TestIgnoreHealthError is failing here.

The structure of the test is like this:
We will have a NOT_SERVING tablet.
We then execute vtctl cmd IgnoreHealthError zone1-slaveuid .*no slave status.*
Ideally, this should ignore the slave status and show us that the tablet is SERVING
Which is not working for me.

The difference between Python and GO is:
In Go, I have shutdown the mysql forcing tablet to go in NOT_SERVING mode. I have also tried it by shutting down the master.
In Python we are only starting a replica. And without a master the replica will remain in NOT_SERVING mode.

Code pointer: https://github.com/planetscale/vitess/blob/tal_tabletmanager_4/go/test/endtoend/tabletmanager/tablet_health_test.go#L249

@deepthi
Copy link
Member

deepthi commented Dec 31, 2019

@deepthi Need your help to debug why TestIgnoreHealthError is failing here.

The structure of the test is like this:
We will have a NOT_SERVING tablet.
We then execute vtctl cmd IgnoreHealthError zone1-slaveuid .*no slave status.*
Ideally, this should ignore the slave status and show us that the tablet is SERVING
Which is not working for me.

The difference between Python and GO is:
In Go, I have shutdown the mysql forcing tablet to go in NOT_SERVING mode. I have also tried it by shutting down the master.
In Python we are only starting a replica. And without a master the replica will remain in NOT_SERVING mode.

Code pointer: https://github.com/planetscale/vitess/blob/tal_tabletmanager_4/go/test/endtoend/tabletmanager/tablet_health_test.go#L249

If mysql is down, you will get a different error (probably CRServerGone or CRServerLost), not no slave status.
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L219
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/query.go#L34

@ajeetj
Copy link
Contributor Author

ajeetj commented Jan 2, 2020

@deepthi Need your help to debug why TestIgnoreHealthError is failing here.
The structure of the test is like this:
We will have a NOT_SERVING tablet.
We then execute vtctl cmd IgnoreHealthError zone1-slaveuid .*no slave status.*
Ideally, this should ignore the slave status and show us that the tablet is SERVING
Which is not working for me.
The difference between Python and GO is:
In Go, I have shutdown the mysql forcing tablet to go in NOT_SERVING mode. I have also tried it by shutting down the master.
In Python we are only starting a replica. And without a master the replica will remain in NOT_SERVING mode.
Code pointer: https://github.com/planetscale/vitess/blob/tal_tabletmanager_4/go/test/endtoend/tabletmanager/tablet_health_test.go#L249

If mysql is down, you will get a different error (probably CRServerGone or CRServerLost), not no slave status.
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L219
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/query.go#L34

Got it. I have replicated the Python test case scenario and now the tests are passing.

Signed-off-by: Ajeet jain <ajeet@planetscale.com>
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
Comment on lines -258 to +318
func TestIgnoreHealthError(t *testing.T) {
func TestNoMysqlHealthCheck(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that we are not testing the IgnoreHealthError functionality? Is there a different test case that covers it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's just github's view. They usually mess up if the same signature method is added up/down.
The TestIgnoreHealthError is still present at line 249.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi deepthi merged commit 2c1b394 into vitessio:master Jan 3, 2020
@deepthi deepthi deleted the tal_tabletmanager_4 branch January 3, 2020 04:43
systay pushed a commit that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants