Add --stream option to `ls` command #5611

hannahhoward · 2018-10-18T15:27:19Z

Features

Adds the option to stream results from ls -- so that large directories (particularly HAMT dirs) start returning data immediately instead of waiting for the whole DAG to be walked

Fixes #5600
Requires ipfs/go-unixfs#39

Implementation

Update LS Command to use go-ipld-cmds library
Update LS Command with --stream option
- Consume EnumLinksAsync in Ls command instead of synchronous Links method
- Support breaking up of command output. Instead of LsObject having to be a full directory, it can represent the beginning of a directory, one more more links in a directory, or the end of the directory - so that outputs can be CAT together to produce the same combined output

For Discussion

Not mergable until go-unixfs is updated for referenced PR
What is our preferred method for unit testing commands?
Suggested refactors for ls command file? Feels a bit overloaded and complicated

alanshaw · 2018-10-18T15:41:11Z

ping @achingbrain

hannahhoward · 2018-10-30T20:43:40Z

Rebased on master, pending updated go-unixfs. However, in the meantime, ready for review @eingenito @Stebalien @magik6k

magik6k · 2018-11-01T10:22:39Z

Some imports are broken, see jenkins. Try running gx-go rw - - fix && gx-go rw

hannahhoward · 2018-11-01T18:50:36Z

@magik6k the issue is while the code for this is merged into go-unixfs, no new version is published. Is there an appropriate process to update and publish a subpackage like go-unixfs?

magik6k · 2018-11-01T20:11:44Z

You just run gx release [patch/minor/major], pin the hash somewhere (like using pinbot), push.

Then if the thing you are trying to update has dependencies, run gx-workspace bubble-list [package-name] in go-ipfs directory and for each dependency open a PR with updates (gx update QmDependency for each changed dependency), and any changes if necessary.

Alternatively, if there are no breaking/few changes and more than say 2-3 dependencies you should try gx-workspace update (see its --help for more).

Stebalien · 2018-11-02T21:14:00Z

This is now unblocked. Can someone review it?

kevina

I am not really happy with all the extra flags and would like to find another way if possible.

core/commands/ls.go

kevina · 2018-11-05T17:14:54Z

@Stebalien This is now blocking #5464. Could we get #5663 in first or separate out the switch to a new commands library into a separate p.r.

kevina · 2018-11-05T18:31:26Z

If required I would be happy to either separate out the command lib. stuff in a separate commit or rebase this on top of #5663. It shouldn't be that hard and I am not sure I am happy with how things are implemented right now so it may take awhile to get this in.

core/commands/ls.go

uses single flag to support state needed by PostRun supports encoding=text License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

hannahhoward · 2018-11-16T23:08:23Z

@Stebalien this incorporates @kevina 's last round of suggestions and I think is ready to merge, pending your approval.

kevina

LGTM now.

There is one comment by @magik6k that I think should be addressed, but we can also clean that up later.

kevina · 2018-11-17T04:01:47Z

core/commands/ls.go

+
+func makeDagNodeLinkResults(req *cmds.Request, dagnode ipld.Node) <-chan unixfs.LinkResult {
+	linkResults := make(chan unixfs.LinkResult)
+	go func() {


I agree, but we can clean this up afterwards.

hannahhoward · 2018-11-19T18:47:28Z

@kevina did cleanup suggested by @magik6k. thank you for the reminder

core/commands/ls.go

kevina

See comment.

License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

Remaining issue is minor.

kevina · 2018-11-20T21:01:42Z

@Stebalien there is one minor issue (keeping me from approving) but otherwise this LGTM.

Stebalien · 2018-11-21T07:45:24Z

Thank @hannahhoward and everyone that reviewed this. Finding a solution when there is no good solution is painful and difficult but you all managed to do it.

🎉

hannahhoward · 2018-12-01T01:47:07Z

Thanks @Stebalien and @kevina and @magik6k for all the feedback!

N.b will not actually do any streaming until ipfs/kubo#5611 lands

N.b will not actually do any streaming until ipfs/kubo#5611 is released.

The only mfs ls method at the moment buffers the output into an array before returning it to the user. This PR adds two new methods, lsPullStream and lsReadableStream to allow the user to either buffer the output themseleves (in case they need sorting, etc) or just stream it on to an output of some sort. N.b the http API will not actually do any streaming until ipfs/kubo#5611 is released.

N.b will not actually do any streaming until ipfs/kubo#5611 lands

The only mfs ls method at the moment buffers the output into an array before returning it to the user. This PR adds two new methods, lsPullStream and lsReadableStream to allow the user to either buffer the output themseleves (in case they need sorting, etc) or just stream it on to an output of some sort. N.b the http API will not actually do any streaming until ipfs/kubo#5611 is released.

hannahhoward requested a review from Kubuxu as a code owner October 18, 2018 15:27

ghost assigned hannahhoward Oct 18, 2018

ghost added the status/in-progress In progress label Oct 18, 2018

hannahhoward requested review from eingenito, magik6k, Stebalien and michaelavila October 18, 2018 15:29

hannahhoward force-pushed the features/streaming-ls-5600 branch from 7dadaa5 to 557630a Compare October 18, 2018 15:49

Stebalien mentioned this pull request Oct 26, 2018

Commands Library Upgrade #5664

Closed

73 tasks

This was referenced Oct 30, 2018

feat(Directory): Add EnumLinksAsync method ipfs/go-unixfs#39

Merged

commands/ls: use new cmds lib #5663

Closed

hannahhoward force-pushed the features/streaming-ls-5600 branch from 557630a to 3b19e7e Compare October 30, 2018 20:42

hannahhoward force-pushed the features/streaming-ls-5600 branch 2 times, most recently from 8033722 to a4e3897 Compare November 1, 2018 21:19

Stebalien force-pushed the features/streaming-ls-5600 branch from a4e3897 to 15707d7 Compare November 2, 2018 21:13

ghost assigned Stebalien Nov 2, 2018

kevina self-requested a review November 2, 2018 21:24

kevina reviewed Nov 2, 2018

View reviewed changes

core/commands/ls.go Outdated Show resolved Hide resolved

core/commands/ls.go Outdated Show resolved Hide resolved

core/commands/ls.go Outdated Show resolved Hide resolved

core/commands/ls.go Outdated Show resolved Hide resolved

schomatis reviewed Nov 3, 2018

View reviewed changes

core/commands/ls.go Outdated Show resolved Hide resolved

schomatis reviewed Nov 3, 2018

View reviewed changes

core/commands/ls.go Outdated Show resolved Hide resolved

Stebalien mentioned this pull request Nov 5, 2018

Add global option to specify the multibase encoding. #5464

Closed

Stebalien reviewed Nov 6, 2018

View reviewed changes

core/commands/ls.go Outdated Show resolved Hide resolved

magik6k and others added 3 commits November 16, 2018 14:53

fix(commands): reduce complexity w/ typed encoder

7391cfd

uses single flag to support state needed by PostRun supports encoding=text License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

chore(commands): Mark feature as experimental

be4c4ab

License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

fix(commands): Use post-run to remove flag

b0dc73c

License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

hannahhoward force-pushed the features/streaming-ls-5600 branch from b756376 to b0dc73c Compare November 16, 2018 23:07

kevina approved these changes Nov 17, 2018

View reviewed changes

kevina reviewed Nov 19, 2018

View reviewed changes

core/commands/ls.go Show resolved Hide resolved

kevina previously requested changes Nov 19, 2018

View reviewed changes

hannahhoward force-pushed the features/streaming-ls-5600 branch from bcb9d26 to 0a5b1a2 Compare November 19, 2018 22:44

refactor(commands): cleanup makeDagNodeLinkResults

f5ab6a3

License: MIT Signed-off-by: hannahhoward <hannah@hannahhoward.net>

hannahhoward force-pushed the features/streaming-ls-5600 branch from 0a5b1a2 to f5ab6a3 Compare November 20, 2018 00:46

magik6k approved these changes Nov 21, 2018

View reviewed changes

Stebalien approved these changes Nov 21, 2018

View reviewed changes

Stebalien merged commit b7a4853 into master Nov 21, 2018

ghost removed the status/in-progress In progress label Nov 21, 2018

Stebalien added status/in-progress In progress and removed blocked by steb labels Nov 21, 2018

Stebalien mentioned this pull request Nov 30, 2018

Implement a streaming ls api #4882

Closed

achingbrain added a commit to ipfs-inactive/js-ipfs-http-client that referenced this pull request Dec 4, 2018

feat: add files.ls streaming methods

fc8c62a

N.b will not actually do any streaming until ipfs/kubo#5611 lands

achingbrain mentioned this pull request Dec 4, 2018

feat: add files.ls streaming methods ipfs-inactive/js-ipfs-http-client#903

Merged

achingbrain added a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Dec 4, 2018

feat: add files.ls streaming methods

2cad7dd

N.b will not actually do any streaming until ipfs/kubo#5611 is released.

achingbrain added a commit to ipfs-inactive/js-ipfs-http-client that referenced this pull request Dec 4, 2018

feat: add files.ls streaming methods

c203ede

N.b will not actually do any streaming until ipfs/kubo#5611 lands

Stebalien deleted the features/streaming-ls-5600 branch February 28, 2019 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --stream option to `ls` command #5611

Add --stream option to `ls` command #5611

hannahhoward commented Oct 18, 2018

alanshaw commented Oct 18, 2018

hannahhoward commented Oct 30, 2018

magik6k commented Nov 1, 2018

hannahhoward commented Nov 1, 2018

magik6k commented Nov 1, 2018

Stebalien commented Nov 2, 2018

kevina left a comment

kevina commented Nov 5, 2018

kevina commented Nov 5, 2018 •

edited

Loading

hannahhoward commented Nov 16, 2018

kevina left a comment

kevina Nov 17, 2018

hannahhoward commented Nov 19, 2018

kevina left a comment

kevina commented Nov 20, 2018

Stebalien commented Nov 21, 2018

hannahhoward commented Dec 1, 2018

Add --stream option to ls command #5611

Add --stream option to ls command #5611

Conversation

hannahhoward commented Oct 18, 2018

Features

Implementation

For Discussion

alanshaw commented Oct 18, 2018

hannahhoward commented Oct 30, 2018

magik6k commented Nov 1, 2018

hannahhoward commented Nov 1, 2018

magik6k commented Nov 1, 2018

Stebalien commented Nov 2, 2018

kevina left a comment

Choose a reason for hiding this comment

kevina commented Nov 5, 2018

kevina commented Nov 5, 2018 • edited Loading

hannahhoward commented Nov 16, 2018

kevina left a comment

Choose a reason for hiding this comment

kevina Nov 17, 2018

Choose a reason for hiding this comment

hannahhoward commented Nov 19, 2018

kevina left a comment

Choose a reason for hiding this comment

kevina commented Nov 20, 2018

Stebalien commented Nov 21, 2018

hannahhoward commented Dec 1, 2018

Add --stream option to `ls` command #5611

Add --stream option to `ls` command #5611

kevina commented Nov 5, 2018 •

edited

Loading