-
Notifications
You must be signed in to change notification settings - Fork 10
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
Vertical split testcase migrated in go #42
Conversation
* pick latest fix from cluster_for_test Signed-off-by: Arindam Nayak <[email protected]>
* tabletmanager lock unlock table test case Signed-off-by: Ajeet jain <[email protected]>
* converted vtctld_test.py to go Signed-off-by: Arindam Nayak <[email protected]>
* migrated one of testcase from schema.py to schema_test.go Signed-off-by: Arindam Nayak <[email protected]>
* converted tabletmanager test cases to go Signed-off-by: Ajeet jain <[email protected]>
* ported testcase of keyspace_test.py Signed-off-by: Arindam Nayak <[email protected]>
* Converted sharded test from py to go Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
* pick latest fix from cluster_for_test Signed-off-by: Arindam Nayak <[email protected]>
* tabletmanager lock unlock table test case Signed-off-by: Ajeet jain <[email protected]>
* converted vtctld_test.py to go Signed-off-by: Arindam Nayak <[email protected]>
* migrated one of testcase from schema.py to schema_test.go Signed-off-by: Arindam Nayak <[email protected]>
* converted tabletmanager test cases to go Signed-off-by: Ajeet jain <[email protected]>
* ported testcase of keyspace_test.py Signed-off-by: Arindam Nayak <[email protected]>
* Converted sharded test from py to go Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: saurabh <[email protected]>
Signed-off-by: saurabh <[email protected]>
Signed-off-by: saurabh <[email protected]>
Signed-off-by: saurabh <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: saurabh <[email protected]>
* moved sql start to non-blocking mode Signed-off-by: Arindam Nayak <[email protected]> Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Ajeet jain <[email protected]> * readme for go endtoend test cases Signed-off-by: Ajeet jain <[email protected]> * Update README.md
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: Arindam Nayak <[email protected]>
Signed-off-by: saurabh <[email protected]>
@deepthi @ajeetj one of the assertion i.e https://github.com/planetscale/vitess/blob/tal_vsplit_tests/go/test/endtoend/sharding/verticalsplit/vertical_split_test.go#L554 is failing.I am looking into it. |
Signed-off-by: saurabh <[email protected]>
All assertions are now working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MInor comments, RLGTM
resultMap := make(map[string]interface{}) | ||
resp, err := http.Get(vtgate.VerifyURL) | ||
if err != nil { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should atleast log the error as we are not throwing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
respByte, _ := ioutil.ReadAll(resp.Body) | ||
err := json.Unmarshal(respByte, &resultMap) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not do panic, log error and return nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
|
||
func TestVerticalSplit(t *testing.T) { | ||
// setting RBR mode | ||
os.Setenv("EXTRA_MY_CNF", path.Join(os.Getenv("VTROOT"), "config", "mycnf", "rbr.cnf")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify, after using the latest code we should not need this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
err = clusterInstance.TopoProcess.ManageTopoDir("mkdir", "/vitess/"+"test_ca") | ||
err = clusterInstance.VtctlProcess.AddCellInfo("test_ca") | ||
err = clusterInstance.TopoProcess.ManageTopoDir("mkdir", "/vitess/"+"test_ny") | ||
err = clusterInstance.VtctlProcess.AddCellInfo("test_ny") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should do assert.Nil(t.err) for all the above statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
|
||
// rdOnly_2 tablet init | ||
_ = clusterInstance.VtctlclientProcess.InitTablet(&sourceRdOnlyTablet2, cellj, sourceKeyspace, hostname, sourceShard.Name) | ||
_ = sourceRdOnlyTablet2.VttabletProcess.CreateDB(sourceKeyspace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- catch and assert error for all the above
- you can also loop over a list of tablets as that the only parameter that is changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
if err != nil { | ||
t.Fatal(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.Nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
_ = clusterInstance.VtctlclientProcess.ExecuteCommand("ApplySchema", "--sql=drop table "+t, "source_keyspace") | ||
} | ||
for _, t := range []cluster.Vttablet{sourceMasterTablet, sourceReplicaTablet, sourceRdOnlyTablet1, sourceRdOnlyTablet2} { | ||
_ = clusterInstance.VtctlclientProcess.ExecuteCommand("ReloadSchema", t.Alias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catch the error, and assert Nil for above 2 places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
resultCountStr := fmt.Sprintf("%v", reflect.ValueOf(resultTableMap["Count"])) | ||
assert.Equal(t, "2", resultCountStr, fmt.Sprintf("unexpected value for VttabletCall(Execute.source_keyspace.0.replica) inside %s", resultCountStr)) | ||
|
||
// getting ['VtgateApi']['Histograms']['ExecuteKeyRanges.destination_keyspace.master']['Count'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unrequired code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
|
||
func checkStats(t *testing.T) { | ||
|
||
//getting ['VttabletCall']['Histograms']['Execute.source_keyspace.0.replica']['Count'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unrequired code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
tabletUID := clusterInstance.GetAndReserveTabletUID() | ||
tablet := &cluster.Vttablet{ | ||
TabletUID: tabletUID, | ||
HTTPPort: clusterInstance.GetAndReservePort(), | ||
GrpcPort: clusterInstance.GetAndReservePort(), | ||
MySQLPort: clusterInstance.GetAndReservePort(), | ||
Alias: fmt.Sprintf("%s-%010d", clusterInstance.Cell, tabletUID), | ||
} | ||
if i == 0 { // Make the first one as master | ||
tablet.Type = "replica" | ||
} else if i == 1 { | ||
tablet.Type = "replica" | ||
} else { | ||
tablet.Type = "rdonly" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use vttablet.GetVttabletInstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajeetj seems to have already done a comprehensive review. I have just a couple of nits to add to that.
@@ -131,7 +132,7 @@ func (vtgate *VtgateProcess) WaitForStatus() bool { | |||
} | |||
|
|||
// GetStatusForTabletOfShard function gets status for a specific tablet of a shard in keyspace | |||
func (vtgate *VtgateProcess) GetStatusForTabletOfShard(name string) bool { | |||
func (vtgate *VtgateProcess) GetStatusForTabletOfShard(name string, count int) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this count for? it is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count is "how many endpoints to wait for"
Since there are 2 rdOnly tablets we need to wait for 2(count) endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then this should be WaitFor...
instead of GetStatusFor..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetStatusForTabletOfShard(vtgate_process.go) is called mostly in conjuction with WaitForStatusOfTabletInShard (vtgate_process.go)) where wait for endpoint is mentioned. I will make a update comment in GetStatusForTabletOfShard to label it as : number of endpoints.
Done. Refer PR46
_ = clusterInstance.VtctlclientProcess.InitTablet(&sourceReplicaTablet, cellj, sourceKeyspace, hostname, sourceShard.Name) | ||
_ = sourceReplicaTablet.VttabletProcess.CreateDB(sourceKeyspace) | ||
|
||
// rdOnly_1 tablet init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in vitess lingo rdonly is one word. Let's also stop using _
, that is python style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refer PR46
Signed-off-by: saurabh <[email protected]>
closing it as new branch created with review comments addressed. |
Vertical split testcases migrated in go.
Signed-off-by: saurabh [email protected]