feat(shard-distributor): refactor time handling, data store structures, key building in etcd #7447
Conversation
arzonus
requested review from
3vilhamster,
Shaddoll,
davidporter-id-au,
demirkayaender,
dkrotx,
jakobht,
neil-xie,
sankari165,
shijiesheng and
taylanisikdemir
as code owners
arzonus
force-pushed
the
CDNC-16878-sd-refactor-time-in-storage
branch
2 times, most recently
from
283bbac to
105d4a5
Compare
arzonus
changed the title
arzonus
changed the title
arzonus
force-pushed
the
CDNC-16878-sd-refactor-time-in-storage
branch
from
cd6edde to
f1dc7cb
Compare
arzonus
changed the title
arzonus
force-pushed
the
CDNC-16878-sd-refactor-time-in-storage
branch
from
f1dc7cb to
00b2219
Compare
arzonus
force-pushed
the
CDNC-16878-sd-refactor-time-in-storage
branch
from
ffeb52c to
66ce47e
Compare
jakobht
left a comment
jakobht
left a comment
There was a problem hiding this comment.
Looks good, left a few questions
| import "fmt" | ||
|
|
||
| //go:generate enumer -type=ExecutorStatus,ShardStatus,AssignmentStatus,MigrationMode -json -output sharddistributor_statuses_enumer_generated.go | ||
| //go:generate enumer -type=ExecutorStatus,ShardStatus,AssignmentStatus,MigrationMode -trimprefix=ExecutorStatus,ShardStatus,AssignmentStatus,MigrationMode -json -output sharddistributor_statuses_enumer_generated.go |
There was a problem hiding this comment.
I guess this changes the DB data, is there something we should be cautious about when deploying (we are still prototyping, so now is the time to do it, but is there anything we need to do?)
There was a problem hiding this comment.
Yeah, it's going to change the DB data, and I agree it's the best time to do it :)
One of the reasons that all our data stored in ETCD contains a type prefix, for example: AssignmentStatusREADY was stored as AssignmentStatusREADY, but not like READY. For me, it was a bit confusing, so I assumed it wasn't done in purpose and was just a typ,o and we should fix it now.
I haven't tested this in terms of backward compatibility, so I assume that SD may fail and it will not work with the existing data stored in etcd, so perhaps we need to clean up the database before deploying.
| } | ||
|
|
||
| isActive := executor.Status == types.ExecutorStatusACTIVE | ||
| isNotStale := (now - executor.LastHeartbeat) <= shardStatsTTL |
There was a problem hiding this comment.
I see we are changing shardStatsTTL to hearbeatTTL - I think the shardStatsTTL was introduced to ensure we didn't do it too often. Just want to make sure this is changed on purpose :)
There was a problem hiding this comment.
Yeah, I just removed the conversion from Time to seconds. Don't think we need to keep a copy of cfg.HeartbeatTTL :)
2 participants
What changed?
RFC3339Nanohas been introduced for time storing to increase precision for timeetcdtypespackage has been created for data structures stored in etcdetcdkeyspackage has been updated, redundant error returns have been removed from build functions, comments have been added, new constants have been addedWhy?
int64was used to save time in Unix seconds format. However, this precision was not enough for measuring latencies. A new time type usesRFC3339Nano2006-01-02T15:04:05.999999999Z07:00and has been introduced to keep consistency during time handling in etcd.How did you test it?
Potential risks
Release notes
Documentation Changes