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

Merge Sharding tests in Go migrated from Python #5790

Merged
merged 5 commits into from
Feb 6, 2020

Conversation

ajeetj
Copy link
Contributor

@ajeetj ajeetj commented Feb 5, 2020

  • The base structure is copied from resharding test case
  • Minor changes in base_sharding.go as we are comparing multiple values now
  • updated config for merge sharding

@ajeetj ajeetj requested a review from sougou as a code owner February 5, 2020 11:13
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.

A few nits. Otherwise looks good. I can approve once they are addressed.

go/test/endtoend/sharding/base_sharding.go Show resolved Hide resolved

// TestV3MergeShardingString - tests merge sharding using a Byte column
func TestV3MergeShardingString(t *testing.T) {
sharding.TestMergesharding(t, true)
Copy link
Member

Choose a reason for hiding this comment

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

it will be nice to add /* useVarbinaryShardingKeyType */ before true.
Also why v3 in the name? v2 is no longer relevant, so we can remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

shard2 = &cluster.Shard{Name: "80-"}

// merge shard
// merging -40 & 40-80 t0 -80
Copy link
Member

Choose a reason for hiding this comment

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

type: t0 -> to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)

// TestV3MergeSharding - tests merge sharding using a INT column
func TestV3MergeSharding(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.

same comment here. remove v3, add comment before boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

test/config.json Outdated
@@ -318,6 +296,28 @@
"site_test"
]
},
"merge_sharding": {
"File": "mergesharding_v3_test.go",
Copy link
Member

Choose a reason for hiding this comment

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

rename v3 -> int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Signed-off-by: Ajeet jain <[email protected]>
@ajeetj
Copy link
Contributor Author

ajeetj commented Feb 6, 2020

@deepthi Addressed review comments. Waiting for checks to pass.

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.

Nice work!

@deepthi deepthi merged commit ebe6d45 into vitessio:master Feb 6, 2020
@deepthi deepthi deleted the tal_merge_sharding branch February 6, 2020 22:34
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