flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

[Access] Add support for pebbleDB to execution data tracker/pruner

Open UlyanaAndrukhiv opened this issue 1 year ago • 3 comments

Closes: #6260

Context

In this pull request:

  • Split out ExecutionDataTracker DB code into common ExecutionDataTracker interface.
  • Refactored badger implementation.
  • Added the pebble version of the storage object.
  • Added functional and integration tests for pebble version of execution data pruning.

UlyanaAndrukhiv avatar Jul 29 '24 20:07 UlyanaAndrukhiv

Codecov Report

Attention: Patch coverage is 39.39394% with 340 lines in your changes missing coverage. Please review.

Project coverage is 41.40%. Comparing base (9653906) to head (3362d0b).

Files with missing lines Patch % Lines
storage/badger/execution_data_tracker.go 65.43% 35 Missing and 21 partials :warning:
storage/pebble/execution_data_tracker.go 63.30% 31 Missing and 20 partials :warning:
storage/pebble/operation/execution_data_tracker.go 0.00% 30 Missing :warning:
storage/badger/operation/execution_data_tracker.go 0.00% 27 Missing :warning:
storage/pebble/operation/common.go 36.58% 25 Missing and 1 partial :warning:
storage/mock/track_blobs_fn.go 0.00% 21 Missing :warning:
cmd/access/node_builder/access_node_builder.go 0.00% 16 Missing :warning:
cmd/observer/node_builder/observer_builder.go 0.00% 16 Missing :warning:
storage/execution_data_tracker.go 37.50% 13 Missing and 2 partials :warning:
storage/mock/prune_callback.go 0.00% 15 Missing :warning:
... and 11 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6277      +/-   ##
==========================================
- Coverage   41.42%   41.40%   -0.03%     
==========================================
  Files        2024     2032       +8     
  Lines      144439   144742     +303     
==========================================
+ Hits        59839    59928      +89     
- Misses      78403    78624     +221     
+ Partials     6197     6190       -7     
Flag Coverage Δ
unittests 41.40% <39.39%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 29 '24 20:07 codecov-commenter

@zhangchiqing since you've been working closely with pebble/badger lately, can you help review this one. It's refactoring the execution data pruner to support both badger and pebble

peterargue avatar Aug 16 '24 20:08 peterargue

Great work! @UlyanaAndrukhiv

Since we're adding both Pebble and Badger implementations for the execution data tracker, this PR has become quite extensive. I suggest we start with the Badger solution first, focusing on the implementation for the tracker and the pruning logic.

This PR also references the Pebble implementation I created in this pull request. However, that PR is still under review, and there's a high likelihood that the Pebble implementation will need refactoring. The patterns we're referring to here might become outdated, so it's better to wait and postpone the Pebble implementation for now.

We could initially implement the pruner in Badger, refactoring it to use Badger batch updates instead of transactions. Since Pebble doesn't support transactions, using Badger batch updates could make it easier for us to switch to the Pebble implementation later.

Design

We might need to revisit the design of the Tracker and Pruner. The original tracker and pruner were implemented a long time ago, and they face several challenges if we switch to Badger batch updates. We should take a step back and reconsider the design first.

For example: For each height, we index a list of CIDs as execution data. CIDs are nested, meaning a CID could have multiple children, and a child CID might have multiple different parent CIDs. When we prune a height, we can't just remove all the CIDs and their child CIDs indexed by that height, because some CID might be referenced by other CIDs at higher height. This is challenging because it requires extra information to determine if a CID or a child CID is prunable.

We tried to address this by keeping track of the highest indexed height for each CID (RetrieveTrackerLatestHeight / UpsertTrackerLatestHeight), but this introduces complexity, and I'm unsure if it's concurrency-safe without database transactions. Is it possible to eliminate the extra index to simplify things? If we keep the LatestHeight index for each CID, we need to be cautious about dirty writes that might corrupt data. For instance, while we're pruning a CID, we might also be concurrently indexing a new height with a certain CID referring to the deleted CID, which could corrupt the newly indexed data. We probably don't want to solve this problem by blocking indexing with a lock during pruning, because pruning might take a long time.

Therefore, I think we need to address these challenges before proceeding with the implementation.

zhangchiqing avatar Sep 13 '24 21:09 zhangchiqing