Implement leveldb storage for dataposter #1736
Conversation
cla-bot
bot
added
the
s
anodar
changed the title
…re already in their own packages)
…from leveldb package
…re already in their own packages)
| func (s *Storage[Item]) GetLast(ctx context.Context) (*Item, error) { | ||
| lastItemIdx, err := s.lastItemIdx(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting last item index: %w", err) |
There was a problem hiding this comment.
I think this should return nil, nil if the last item index is missing (i.e. there are no items). We should also think about what happens when all items are pruned. In that case, we probably want to have pruning remove the last item index from the db.
There was a problem hiding this comment.
Done. Checking length, if it's 0 then returning nil, nil. So no need to remove last item idx from db when pruning (for all public methods behavior will be exactly the same).
| } | ||
| if err := b.Put(countKey, []byte(strconv.Itoa(cnt+1))); err != nil { | ||
| return fmt.Errorf("updating length counter: %w", err) | ||
| if bytes.Compare(key, lastItemIdx) > 0 { |
There was a problem hiding this comment.
I don't love assuming the return value of lastItemIdx is empty if leveldb.ErrNotFound is returned. In practice I'm sure it'll be empty, but I don't think it's good Go practice to read the value returned in an error is present.
| @@ -71,6 +71,8 @@ func (s *Storage[Item]) Put(_ context.Context, index uint64, prevItem *Item, new | |||
| if queueIdx > len(s.queue) { | |||
| return fmt.Errorf("attempted to set out-of-bounds index %v in queue starting at %v of length %v", index, s.firstNonce, len(s.queue)) | |||
| } | |||
| // TODO: come up with a better way of comparing prev/new values, | |||
| // we shouldn't use cmp.Diff outside tests. | |||
| if diff := cmp.Diff(prevItem, s.queue[queueIdx]); diff != "" { | |||
There was a problem hiding this comment.
FYI the original code did intentionally use pointer comparisons, because that worked with the batch poster's design. But I agree it isn't ideal. We should either RLP encode both and compare them here, or we could use reflect.DeepEqual.
anodar
dismissed
PlasmaPower’s stale review
The merge-base changed after approval.
No description provided.