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

Streamed pins #290

Merged
merged 6 commits into from
Jun 2, 2023
Merged

Streamed pins #290

merged 6 commits into from
Jun 2, 2023

Conversation

MichaelMure
Copy link
Contributor

@MichaelMure MichaelMure commented May 5, 2023

Corresponding kubo PR: ipfs/kubo#9859

The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Also in this PR, a DAG generator to simplify testing (used in the kubo PR to test the GC).

replace ipfs/interface-go-ipfs-core#109
replace ipfs/go-ipfs-pinner#24
replace ipfs/go-ipfs-provider#48
replace ipfs/go-ipfs-blocksutil#18

@MichaelMure MichaelMure requested a review from a team as a code owner May 5, 2023 14:54
@welcome
Copy link

welcome bot commented May 5, 2023

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

MichaelMure added a commit to MichaelMure/go-ipfs that referenced this pull request May 5, 2023
See ipfs/boxo#290

This PR follow the changes in the Pinner to make listing recursive and direct pins asynchronous, which in turns allow pin/ls to build and emit results without having to wait anything, or accumulate too much in memory.

Note: there is a tradeoff for pin/ls?type=all:
- keep the recursive pins in memory (which I chose)
- ask the pinner twice for the recursive pins, and limit memory usage

Also, follow the changes in the GC with similar benefit of not having to wait the full pin list. Add a test.
Also, follow the changes in pin.Verify.
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #290 (2931a8e) into main (4c5c98b) will increase coverage by 0.04%.
The diff coverage is 52.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
+ Coverage   49.51%   49.56%   +0.04%     
==========================================
  Files         282      283       +1     
  Lines       33836    33876      +40     
==========================================
+ Hits        16755    16791      +36     
- Misses      15316    15322       +6     
+ Partials     1765     1763       -2     
Impacted Files Coverage Δ
coreiface/tests/pin.go 0.00% <0.00%> (ø)
pinning/pinner/pin.go 32.35% <ø> (ø)
provider/simple/reprovide.go 0.00% <0.00%> (ø)
ipld/merkledag/test/dag_generator.go 54.34% <54.34%> (ø)
pinning/pinner/dspinner/pin.go 58.50% <77.14%> (+0.76%) ⬆️

... and 15 files with indirect coverage changes

MichaelMure added a commit to MichaelMure/go-ipfs that referenced this pull request May 5, 2023
See ipfs/boxo#290

This PR follow the changes in the Pinner to make listing recursive and direct pins asynchronous, which in turns allow pin/ls to build and emit results without having to wait anything, or accumulate too much in memory.

Note: there is a tradeoff for pin/ls?type=all:
- keep the recursive pins in memory (which I chose)
- ask the pinner twice for the recursive pins, and limit memory usage

Also, follow the changes in the GC with similar benefit of not having to wait the full pin list. Add a test.
Also, follow the changes in pin.Verify.
@lidel lidel requested a review from Jorropo May 15, 2023 13:53
@BigLep
Copy link
Contributor

BigLep commented May 25, 2023

@MichaelMure : I know this has been open for awhile. Apologies. Maintainers spoke on 2023-05-25. We'll make sure this goes out with Kubo 0.21.

Slack context: https://filecoinproject.slack.com/archives/C03L0G3B4RX/p1684507196595209

@BigLep BigLep mentioned this pull request May 31, 2023
5 tasks
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

I'll do the rebase on main, LGTM.

coreiface/tests/pin.go Outdated Show resolved Hide resolved
pinning/pinner/dspinner/pin.go Outdated Show resolved Hide resolved
pinning/pinner/dspinner/pin_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM I'll test on Kubo

@Jorropo Jorropo self-assigned this Jun 2, 2023
Jorropo pushed a commit to MichaelMure/go-ipfs that referenced this pull request Jun 2, 2023
See ipfs/boxo#290

This PR follow the changes in the Pinner to make listing recursive and direct pins asynchronous, which in turns allow pin/ls to build and emit results without having to wait anything, or accumulate too much in memory.

Note: there is a tradeoff for pin/ls?type=all:
- keep the recursive pins in memory (which I chose)
- ask the pinner twice for the recursive pins, and limit memory usage

Also, follow the changes in the GC with similar benefit of not having to wait the full pin list. Add a test.
Also, follow the changes in pin.Verify.
@hacdias
Copy link
Member

hacdias commented Jun 2, 2023

@Jorropo what's the status?

MichaelMure and others added 6 commits June 2, 2023 14:27
The rational is that if the pin list get big, a synchronous call to get the complete list can delay handling unnecessarily. For example, when listing indirect pins, you can start walking the DAGs immediately with the first recursive pin instead of waiting for the full list.

This matters even more on low power device, of if the pin list is stored remotely.

Replace:
- ipfs/go-ipfs-pinner#24
- ipfs/go-ipfs-provider#48
Rationale is that generating a test DAG is quite difficult, and anything that helps writing better tests is helpful.

Replace ipfs/go-ipfs-blocksutil#18
Jorropo pushed a commit to MichaelMure/go-ipfs that referenced this pull request Jun 2, 2023
See ipfs/boxo#290

This PR follow the changes in the Pinner to make listing recursive and direct pins asynchronous, which in turns allow pin/ls to build and emit results without having to wait anything, or accumulate too much in memory.

Note: there is a tradeoff for pin/ls?type=all:
- keep the recursive pins in memory (which I chose)
- ask the pinner twice for the recursive pins, and limit memory usage

Also, follow the changes in the GC with similar benefit of not having to wait the full pin list. Add a test.
Also, follow the changes in pin.Verify.
@Jorropo Jorropo merged commit e2fc7f2 into ipfs:main Jun 2, 2023
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this pull request Jun 19, 2023
See ipfs/boxo#290

This PR follow the changes in the Pinner to make listing recursive and direct pins asynchronous, which in turns allow pin/ls to build and emit results without having to wait anything, or accumulate too much in memory.

Note: there is a tradeoff for pin/ls?type=all:
- keep the recursive pins in memory (which I chose)
- ask the pinner twice for the recursive pins, and limit memory usage

Also, follow the changes in the GC with similar benefit of not having to wait the full pin list. Add a test.
Also, follow the changes in pin.Verify.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants