Add simple byte-counting export progress #7037
Conversation
ribasushi
force-pushed
the
feat/carfile-export-only_with_progress
branch
from
31f8f54 to
f6ae38a
Compare
core/commands/dag/dag.go
Outdated
| } | ||
|
|
||
| if err := func() error { | ||
| bar := pb.New64(0).SetUnits(pb.U_BYTES) |
There was a problem hiding this comment.
How does this look when adding multiple files? It would be nice to see:
Exported Qm...
Exported Qm...
Exported Qm...
[ progress ]
Instead of having dead progress bars in between. But it's not a huge issue.
There was a problem hiding this comment.
How does this look when adding multiple files?
We can only export one root/DAG ( single CID spec, no selectors ). The exporter only sees "bytes written" into a single stream on STDOUT. No files are created.
There was a problem hiding this comment.
I think I see what you mean... let me rewire this / rebase on top of the approved export.
There was a problem hiding this comment.
No you are right - there may be a reason for the server to emit multiple read-ables for some reason. So having the bar outside of the loop make sense. I will redo it.
There was a problem hiding this comment.
That shouldn't happen... and I'm not sure what we'd be able to do about it. Would we just concatenate them?
There was a problem hiding this comment.
Yep, concat.
Alternatively we can throw saying "unexpected multipart export..." or somesuch.
OR we can leave the multi-bar spawning as-is ( no changes to PR ), then there will eb a visual cue "something went wrong"
But if you say "this shouldn't happen" - erroring is what I prefer.
There was a problem hiding this comment.
Addressed as https://github.com/ipfs/go-ipfs/pull/7037/files#diff-15172926c4147422f472139313467179R388-R396
There is more code on the closer, as that became a can of worms, but it will go in the other PR once this is folded in ( I promise it will make sense in a bit )
Waiting for CI to run here, and will merge this into the other branch.
ribasushi
force-pushed
the
feat/carfile-export-only_with_progress
branch
from
f6ae38a to
ce72271
Compare
|
@ribasushi please avoid merging with non-trivial unreviewed changes. The plan is to merge |
|
@Stebalien you did sign off on both the target and the destination... |
|
That signoff is from 20:29 PDT. The discussion ended at 21:19 PDT (#7037 (comment)). I never saw that change. |
|
I went off my mail client, thought you saw the progress bar and then hit approve. My bad. Is there something wrong with https://github.com/ipfs/go-ipfs/pull/7037/files#diff-15172926c4147422f472139313467179R388-R396 ? I'll fix whatever's needed. |
|
Nope, the change looks good. I'm just trying to prevent bugs from slipping through. |
This is the simple, non-controversial, export-progress-meter modeled on how
ipfs catworks. R made agains the base mplementing branch for ease of review.