feat: waku store sync 2.0 storage and tests #3215
Conversation
SionoiS
requested review from
darshankabariya,
gabrielmer,
NagyZoltanPeter and
Ivansete-status
SionoiS
changed the title
|
You can find the image built from this PR at Built from 8535fff |
Yes, I will add some. |
| method insert*( | ||
| self: SyncStorage, element: ID | ||
| ): Result[void, string] {.base, gcsafe, raises: [].} = | ||
| discard |
There was a problem hiding this comment.
Maybe insert should be idempotent?
There was a problem hiding this comment.
I really like the "idempotent" word but, what does it mean in this context ;P ?
There was a problem hiding this comment.
Currently, when inserting with the same element the first call would be ok() but if you call again err() since the element was already present.
An idempotent function would always insert the same element at the same index and would result in no side effect when calling the same func with the same element multiple times.
| method insert*( | ||
| self: SyncStorage, element: ID | ||
| ): Result[void, string] {.base, gcsafe, raises: [].} = | ||
| discard |
There was a problem hiding this comment.
I really like the "idempotent" word but, what does it mean in this context ;P ?
| self: SeqStorage, | ||
| inputBounds: Slice[ID], | ||
| inputFingerprint: Fingerprint, | ||
| output: var SyncPayload, |
There was a problem hiding this comment.
Why not return it inside a Result?
There was a problem hiding this comment.
Well there's no way for the proc to fail.
Also, the var payload could have been local to the proc but the caller has a loop so it's inconvenient.
| # entire range is after any of our elements | ||
| return none(Slice[int]) | ||
|
|
||
| return some(lower ..< upper) |
There was a problem hiding this comment.
Should the returned Slice not include the upper too?
| return some(lower ..< upper) | |
| return some(lower .. upper) |
There was a problem hiding this comment.
🤔 I'm pretty sure that the Slice[ID] exclude the upper bound so we do the same.
| let state = ItemSet(elements: @[], reconciled: true) | ||
| output.itemSets.add(state) |
There was a problem hiding this comment.
I'm a bit puzzled about these two lines :)
Are we trying to "return" an empty SyncPayload?
There was a problem hiding this comment.
Yes it's a valid case where one side have an empty range.
Peer 1:
range A to F
fingerprint 0x53...
Peer 2:
range A to F
no elements :(
send back an empty item set
| let slice = | ||
| if idxSlice.isNone(): | ||
| if not inputItemSet.reconciled: | ||
| output.ranges.add((inputBounds, itemSetRange)) | ||
| let state = ItemSet(elements: @[], reconciled: true) | ||
| output.itemSets.add(state) | ||
| else: | ||
| output.ranges.add((inputBounds, skipRange)) | ||
|
|
||
| return | ||
| else: | ||
| idxSlice.get() |
There was a problem hiding this comment.
I see some room for encapsulation as apparently we have similar approach two or three times
There was a problem hiding this comment.
They are slightly different. One has a return the other continue
Description
Second PR for new Waku store sync 2.0
Include everything Storage related and tests
Specification
Research issue
Changes
N.B. Cannot be merged before #3213
Followed by #3216
Issue