Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user defined block hooks when using two step write for selective cars #37

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

This is a simple but pretty obscure feature. When building cars with Selectors, we have an option for a two step write -- one step performs the traversal, while another writes the traversed cids to a file. This is important among other things so that we can get a total file size ahead of time. We also have an option in the 1-step write to call a user defined hook as we write blocks. Now, we allow the two step write to use these user-defined hooks. However, they are not called till you actually dump the file. For a variety of reasons, we need this for Filecoin, but it seems to me to be a non-controversial enough change as to allow us to merge to master?

Implementation

  • hold the user funcs in the intermediate structure
  • track offset/size and call during io.Write

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #37 (7f9fecd) into master (c2f1ff2) will increase coverage by 0.95%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   50.20%   51.16%   +0.95%     
==========================================
  Files           3        3              
  Lines         245      258      +13     
==========================================
+ Hits          123      132       +9     
- Misses         87       89       +2     
- Partials       35       37       +2     
Impacted Files Coverage Δ
selectivecar.go 65.83% <71.42%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2f1ff2...7f9fecd. Read the comment docs.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

sgtm

there's a loss of coverage here though because the Write() and Prepare() without hook case isn't being covered anymore

@hannahhoward hannahhoward merged commit 8fe0b74 into master Nov 19, 2020
@hannahhoward
Copy link
Collaborator Author

I merged cause there's not a seperate code path for no-hooks -- it's just simpler

@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
@mvdan mvdan deleted the feat/two-step-user-funcs branch June 25, 2021 12:29
Jorropo pushed a commit to ipfs/boxo that referenced this pull request Mar 22, 2023
Allow user defined block hooks when using two step write for selective cars

This commit was moved from ipld/go-car@8fe0b74
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.

2 participants