Skip to content

Conversation

@aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Jan 21, 2022

For IPFS <-> Filecoin interop.

A blockstore that can serve retrieval for any cid across all shards that the dagstore has.

TODO

  • Add fuzzed test.
  • GetSize should use the underlying blockstore and not load the entire block in memory.
  • When a blockstore is removed from the cache it is closed. Use reference counting to ensure that the blockstore is not closed while there is still an outstanding operation against the blockstore

@aarshkshah1992 aarshkshah1992 marked this pull request as draft January 21, 2022 13:58
@aarshkshah1992 aarshkshah1992 changed the title [WIP ]Blockstore on all dagstore cids Blockstore on all dagstore cids Mar 29, 2022
@aarshkshah1992 aarshkshah1992 marked this pull request as ready for review March 29, 2022 05:26
@aarshkshah1992 aarshkshah1992 requested a review from dirkmc March 29, 2022 05:26
Co-authored-by: dirkmc <dirkmdev@gmail.com>
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How and when are shards released?
  • Suggest naming as IndexBackedBlockstore.
  • If possible, put in a different package (ex. indexbs?) as this is a utility and strictly not needed for the operation of the DagStore.
  • How is the LRU cache configured?

@raulk
Copy link
Member

raulk commented Mar 29, 2022

Is there a reason that this needs to be a member of the DagStore?

@aarshkshah1992
Copy link
Contributor Author

@raulk

  • Have renamed it as IndexBackedBlockstore in a new indexbs package.
  • Yeah, I think it dosen't need to be a member of dagstore. Have moved it outside.
  • Shards are released when they are evicted from the lru blockstore cache. See
    bslru, err := lru.NewWithEvict(maxCacheSize, func(_ interface{}, val interface{}) {
    .

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Mar 29, 2022

@raulk @dirkmc

There was a race here where two Get calls for the same shard key create two shard accesors and only one gets added to the lru cache and eventually relased whereas we lose the reference to the other one without ever releasing it (because multiple add calls to the lru cache for the same key only updates the priority of the item).

I've fixed this with striped locking based on shard key to synchornize access to the lru cache.

@aarshkshah1992
Copy link
Contributor Author

@raulk Please can you take a look ?

@MichaelMure
Copy link

May I ask what's the status of this work?

@Wondertan
Copy link

Hello. We would like to use DAGStore together with a top-level index. We also need a Blockstore over the top-level index to serve sampling requests to our light clients, so we would like to have this merged not to reinvent the wheel:

  • What else needs to be done here to say it's done?
  • Can we somehow facilitate the process?

Kindly ping @raulk, @dirkmc, and @aarshkshah1992. I know you are busy with FVM and Boost things, but this should not take too much time from you. Thanks!

@willscott
Copy link
Contributor

@hannahhoward probably has the most recent context here, and I think we are hoping to finish the work needed to get this finished.

@dirkmc
Copy link
Contributor

dirkmc commented Sep 5, 2022

@Wondertan @MichaelMure we are hoping to get it merged this week

@dirkmc dirkmc requested a review from nonsense September 5, 2022 16:57
Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dirkmc dirkmc merged commit 0878b63 into master Sep 30, 2022
@dirkmc dirkmc deleted the feat/blockstore branch September 30, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants