Use common progressbar function for cat and get #1779
Conversation
|
I think this needs rebase. |
|
|
||
| ctx cxt.Context | ||
| } | ||
|
|
||
| // NewWriter wraps given io.Writer. | ||
| func NewWriter(ctx cxt.Context, dag mdag.DAGService, archive bool, compression int, w io.Writer) (*Writer, error) { | ||
| func NewWriter(ctx cxt.Context, dag mdag.DAGService, archive bool, compression int, w io.Writer, progressBarOut io.Writer, size uint64) (*Writer, error) { |
There was a problem hiding this comment.
No, this is not the right way to do this. the prograss bar stuff does not belong here. All this stuff is already complicated, and this makes it way worse. there must be some way to report out the information from the Writer.
The simplicity and cleanliness of Interfaces really matters to their usability.
There was a problem hiding this comment.
and their maintenance. I know the codebase is not great in this regard, but we should always strive to make it better. this progress bar stuff has no business inside unixfs/archive/tar/writer.
There was a problem hiding this comment.
How about adding a channel telling the size being read to DAGService? Though a callback is more generic to allow outputting Object as in the requirement in #1778.
|
@rht let's walk through the problem again. is the issue that the progress cannot be estimated because the output is a tar archive, and tar adds a bunch of overhead? So the proposed solution here is to measure the bytes before they're tarred? some other possible solutions:
|
Yes it is possible to do this accurately for tarred object, it's just that the entire dag has to be traversed. On second thought, the progressbar should only report progress actually piped to the client (the tar.gz-ed stream) that it doesn't make sense to tell the pre-tar.gz stream. |
|
(there is the client, and then there is the local node downloading stream from remote node...) |
hmm yes. forgot dags don't include the total # of objects underneath. :/ |
rht
force-pushed
the
latest-progressbar
branch
from
2ee3b73 to
a73a333
Compare
rht
changed the title
|
Just removed the progressbar on dagreader. |
|
Either:
|
| @@ -159,8 +181,7 @@ func (gw *getWriter) writeArchive(r io.Reader, fpath string) error { | |||
| defer file.Close() | |||
|
|
|||
| fmt.Fprintf(gw.Out, "Saving archive to %s\n", fpath) | |||
| bar, barR := progressBarForReader(gw.Err, r) | |||
| bar.Start() | |||
There was a problem hiding this comment.
we should keep the Start with the Finish so it's symmetric calls. less error prone. think of these three lines like:
lk := sync.Lock{}
lk.Lock()
defer lk.Unlock()
|
👍 to the refactor. though:
|
License: MIT Signed-off-by: rht <rhtbot@gmail.com>
rht
force-pushed
the
latest-progressbar
branch
from
a73a333 to
7caba7e
Compare
Use common progressbar function for cat and get
Where the progressbar wrapper is put before the stream is turned into tar.