fix(store): properly update store head #207
Conversation
cristaloleg
changed the title
| from, to := headers[0].Height(), headers[ln-1].Height() | ||
| if height+1 != from && height != 0 { // height != 0 is needed to enable init from any height and not only 1 |
There was a problem hiding this comment.
Removing this check 'cause we want non-adjacent adds.
There was a problem hiding this comment.
We want non-adjacent adds, but we also want to make sure that to is not lower than from height (meaning headers are sorted by height (lowest --> highest)) before being published
There was a problem hiding this comment.
I know the caller already sorts but it doesn't hurt to keep the check there.
cristaloleg
requested review from
tzdybal,
renaynay,
Wondertan,
vgonkivs,
gupadhyaya,
walldiss and
ramin
as code owners
| head, err := s.GetByHeight(ctx, s.heightSub.Height()) | ||
| if err == nil { | ||
| return head, nil | ||
| headPtr := s.writeHead.Load() |
There was a problem hiding this comment.
One of the biggest change: we always trying to return writeHead here and only when it's not available (read right after .Init and before 1st .Append) we will fetch data from underlying store.
Side effect: Store[H].Head should be faster now but just a bit 'cause GetByHeight uses cache.
cristaloleg
force-pushed
the
store/fix-store-head
branch
from
964e86f
to
a197550
Compare
| from, to := headers[0].Height(), headers[ln-1].Height() | ||
| if height+1 != from && height != 0 { // height != 0 is needed to enable init from any height and not only 1 |
There was a problem hiding this comment.
We want non-adjacent adds, but we also want to make sure that to is not lower than from height (meaning headers are sorted by height (lowest --> highest)) before being published
| from, to := headers[0].Height(), headers[ln-1].Height() | ||
| if height+1 != from && height != 0 { // height != 0 is needed to enable init from any height and not only 1 |
There was a problem hiding this comment.
I know the caller already sorts but it doesn't hurt to keep the check there.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
==========================================
+ Coverage 62.80% 63.82% +1.01%
==========================================
Files 39 38 -1
Lines 3589 3621 +32
==========================================
+ Hits 2254 2311 +57
+ Misses 1160 1141 -19
+ Partials 175 169 -6 ☔ View full report in Codecov by Sentry. |
| from, to := headers[0].Height(), headers[ln-1].Height() | ||
| if height+1 != from && height != 0 { // height != 0 is needed to enable init from any height and not only 1 | ||
| log.Fatalf("PLEASE FILE A BUG REPORT: headers given to the heightSub are in the wrong order: expected %d, got %d", height+1, from) | ||
| return | ||
| if from > to { |
There was a problem hiding this comment.
Are from and to inclusive? For example, is interval like this is [from:10;to:10] is valid and has length 1?
| // TODO(cristaloleg): benchmark this timeout or make it dynamic. | ||
| ctx, cancel := context.WithTimeout(ctx, 10*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
What is the purpose of additional cap of timeout here instead of using original context?
| ok := true | ||
| ok = ok && exp.Height() == have.Height() | ||
| ok = ok && syncer.pending.Head() == nil | ||
|
|
||
| ok = ok && uint64(exp.Height()) == state.Height | ||
| ok = ok && uint64(2) == state.FromHeight | ||
| ok = ok && uint64(exp.Height()) == state.ToHeight | ||
| ok = ok && state.Finished() |
There was a problem hiding this comment.
Looks unusual. Can it be just couple of require.Equal() calls?
Overview
The main idea of this PR is to make
Store[H].Headworking properly, to be precise: returning head that was written to the disk (*). Along with thatheightSub.heightis increased monotonically to prevent bugs when we have store appends out of order.To test everything I'm adding 2 new tests: one that verifies out of order appends and another when which does this concurrently. Which helped to find 2 or even 3 edge cases during coding.
Fixes #201