-
Notifications
You must be signed in to change notification settings - Fork 62
Fix the migrating slot didn't compatible with old format #310
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
Fix the migrating slot didn't compatible with old format #310
Conversation
store/cluster_shard.go
Outdated
) | ||
|
||
const ( | ||
NotMigratingString = "not-migrating" |
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.
Up to suggestions on what string we want here...
@bseto Thanks for your efforts. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #310 +/- ##
============================================
+ Coverage 43.38% 46.76% +3.37%
============================================
Files 37 45 +8
Lines 2971 4386 +1415
============================================
+ Hits 1289 2051 +762
- Misses 1544 2128 +584
- Partials 138 207 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cmd/client/command/helper.go
Outdated
if shard.MigratingSlot != nil { | ||
migratingStatus = fmt.Sprintf("%s --> %d", shard.MigratingSlot, shard.TargetShardIndex) | ||
if shard.MigratingSlot.IsMigrating { | ||
migratingStatus = fmt.Sprintf("%s --> %d", &shard.MigratingSlot.SlotRange, shard.TargetShardIndex) |
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.
It should be good without &
?
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.
woops
store/cluster_shard.go
Outdated
) | ||
|
||
const ( | ||
NotMigratingString = "not-migrating" |
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 could keep the field null if it's not migrating.
@git-hulk I based the PR on your previous PR.
For some additional context: #308 (comment)
Did some manual testing
-1
migrating_slot
valueunstable
branch with thenull
value. (so any clusters created from May 3rd to now will not be compatible)