bseto commented on code in PR #308:
URL:
https://github.com/apache/kvrocks-controller/pull/308#discussion_r2083113223
##########
store/slot.go:
##########
@@ -91,15 +91,30 @@ func (slotRange *SlotRange) MarshalJSON() ([]byte, error) {
}
func (slotRange *SlotRange) UnmarshalJSON(data []byte) error {
- var slotsString string
+ var slotsString any
if err := json.Unmarshal(data, &slotsString); err != nil {
return err
}
- slotObject, err := ParseSlotRange(slotsString)
- if err != nil {
- return err
+ switch t := slotsString.(type) {
+ case string:
+ slotObject, err := ParseSlotRange(t)
+ if err != nil {
+ return err
+ }
+ *slotRange = *slotObject
+ case float64:
+ // We use integer to represent the slot because we don't
support the slot range
+ // in the past. So we need to support the integer type for
backward compatibility.
+ // But the number in JSON is float64, so we need to convert it
to int here.
+ if t < MinSlotID || t > MaxSlotID {
+ return ErrSlotOutOfRange
Review Comment:
Another way we could do things is instead of defining `Shard` with a pointer
to the struct, like this:
https://github.com/apache/kvrocks-controller/blob/3bdddb543c076b197f959a3e6dbcea11d161dca7/store/cluster_shard.go#L40
Maybe we add a new struct that embeds SlotRange that exposes a new field,
maybe something like `migrating` which denotes if it's migrating or not.
We can still do range checks on the start/stop, but if we receive just a
single `-1`, then we set the migrating field to false.
It'll also require it's own marshal/unmarshal, depending on what you want it
to output.
Before it was `-1` to denote not migrating. Right now it's `null`. But maybe
we could make it just a string like
`"not-migrating"` to be more clear, and when we unmarshal "not-migrating",
then it'll also set `isMigrating` to false.
```go
type MigrationSlotRange struct {
SlotRange
isMigrating bool
}
type Shard struct {
Nodes []Node `json:"nodes"`
SlotRanges []SlotRange `json:"slot_ranges"`
TargetShardIndex int `json:"target_shard_index"`
MigratingSlot MigrationSlotRange `json:"migrating_slot"`
}
```
Sorry about this bug, I probably won't have time tomorrow to work on this,
but I have some time on Sunday if you'd like me to make the changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
