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

pin cmd: stream recursive pins #5005

Closed
wants to merge 3 commits into from
Closed

pin cmd: stream recursive pins #5005

wants to merge 3 commits into from

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented May 8, 2018

This is api breaking change.

When doing pin ls on large repos nothing will happen until all pins have been gathered, with this we stream them as we traverse the recursive pin children

@magik6k magik6k requested a review from Kubuxu as a code owner May 8, 2018 23:36
@ghost ghost assigned magik6k May 8, 2018
@ghost ghost added the status/in-progress In progress label May 8, 2018
@Stebalien
Copy link
Member

I take it this now returns the pins as separate objects instead of as {keys: [pins...]}? That definitely sounds like an improvement but breaking things is... unfortunate.

One solution is to add a --stream flag and turn it on by default (on the client). I wish we had a nice, standard way to add features like this.

Thoughts @diasdavid? This change will affect JS as well.

@Stebalien Stebalien requested a review from daviddias May 9, 2018 09:53
@Stebalien Stebalien added the topic/interop Interoperability label May 9, 2018
@daviddias
Copy link
Member

I like this proposal. Before merging, can we test it with js-ipfs-api and make sure that the X-Stream/X-Chunked headers are being well set and that we do get a proper ndjson stream?

@whyrusleeping
Copy link
Member

@diasdavid for sure.

Before making this change we should also get buyin from all users of this api. which is a pretty strong task.

It may also be worth identifying other places where a similar change could make sense to do, and maybe do them all at once

@kevina
Copy link
Contributor

kevina commented Jun 13, 2018

One solution is to add a --stream flag and turn it on by default (on the client). I wish we had a nice, standard way to add features like this.

Yes I would something similar. I have done similar things with other commands I believe.

@whyrusleeping
Copy link
Member

@diasdavid is there a js-ipfs-api test for this that we can easily run?

@daviddias
Copy link
Member

@whyrusleeping running js-ipfs-api tests will run you through these -- https://github.com/ipfs/interface-ipfs-core/blob/master/js/src/pin.js -- which has a test for pin ls with recursive type.

License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
Copy link
Member Author

magik6k commented Jun 20, 2018

So I've tried to move this, but I'm not that familiar with cmds lib:

  • the --stream flag works with curl just fine
  • the Type depends on whether or not the flag is set so I had to leave it out, which is a bit problematic for the Encoder.
    • Should I just treat the data I get there as a unmarshalled json (i.e. try casting to map[string]interface{})
    • I could just emit multiple RefKeyList objects, but the json output would be rather weird.
  • X-Chunked-Output: 1 is now set in headers regardless of whether or not the --stream flag is set. I'm not sure if this is caused by rewrite to the new cmd lib, lack of the Type field or something else.

MichaelMure added a commit to MichaelMure/go-ipfs that referenced this pull request Jul 10, 2019
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
@Stebalien
Copy link
Member

New version in #6493.

@Stebalien Stebalien closed this Jul 10, 2019
@daviddias daviddias deleted the fix/pin-stream branch July 10, 2019 19:14
@daviddias daviddias restored the fix/pin-stream branch July 10, 2019 19:14
lanzafame pushed a commit that referenced this pull request Jul 30, 2019
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of #5005
ralendor pushed a commit to ralendor/go-ipfs that referenced this pull request Jun 6, 2020
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
ralendor pushed a commit to ralendor/go-ipfs that referenced this pull request Jun 6, 2020
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
ralendor pushed a commit to ralendor/go-ipfs that referenced this pull request Jun 8, 2020
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
ralendor pushed a commit to ralendor/go-ipfs that referenced this pull request Jun 8, 2020
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
ralendor pushed a commit to ralendor/go-ipfs that referenced this pull request Jun 8, 2020
Add a --stream flag to stream the results instead of
accumulating the final result in memory.

This is a rework of ipfs#5005
@hacdias hacdias deleted the fix/pin-stream branch May 9, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/in-progress In progress topic/interop Interoperability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants