-
Notifications
You must be signed in to change notification settings - Fork 97
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
Copy progress - feature request #839
Comments
How about a |
What does it look like? How is it better than a channel or a func? Maybe a practical example would help me understand it? |
Something like https://pkg.go.dev/oras.land/oras@v1.2.0/cmd/oras/internal/display/status/progress#Manager type Manager interface {
Add() (Status, error)
Close() error
}
type Status chan *status
// status is used as message to update progress view.
type status struct {
done bool // done is true when the end time is set
prompt string
descriptor ocispec.Descriptor
offset int64
total humanize.Bytes
speedWindow *speedWindow
startTime time.Time
endTime time.Time
mark spinner
lock sync.Mutex
} The Maybe we can improve a bit by separating them. /cc @qweeah |
I truly don't remember that part. 😁 I could see having an What is the If we want to add progress to oras CLI, it becomes a consuming party, I think I would handle it as a separate addition in a separate PR. |
I thought about this a bit more over the last few days. I still am having a hard time seeing the benefit of a := func(msg oras.Update) {
fmt.Printf("copied %d bytes out of %d total for %s\n", msg.Copied, msg.Descriptor.Size, msg.Descriptor.Digest)
}
opts := oras.CopyOptions{
ProgressHandler: f
}
desc, err := oras.Copy(ctx, src, tagName, dst, tagName, opts) With each copy of data, it just calls I have been thinking a bit more about the whole "how do we call the function without blocking". I had suggested earlier:
but I like this less. I am worried about a proliferation of goroutines. If we have frequent updates, that could get out of control. Here is a possible alternative.
This is less of an issue if we pass a channel in the options, instead of a function, but I do think the option is cleaner. Thoughts @shizhMSFT ? |
As discussed in this comment (with thanks to @shizhMSFT for shepherding it through).
Summary
oras.Copy()
Proposed design
The signature for oras.Copy includes oras.CopyOptions as the last parameter. If this were variadic, I would suggest adding another
WithProgressUpdate()
or similar, but we use a singleCopyOptions
, so I propose adding another property:CopyOptions.Progress
. The type depends on the design choice.There are two ways to do this:
CopyOptions.Progress func(Update)
. With each update,Copy()
(or its underlying functions) would call the passed function, passing it theUpdate
(see below).CopyOptions.Progress chan<- Update
. With each updateCopy()
would send anUpdate
to the channelIf
Progress
is nil, then this functionality is bypassed.Despite my linked PR using channels, I have no strong preference for channel over function.
Preventing Blocking
With both functions and channels, there is a concern that it might block. In principle, I think that if someone calls
Copy()
and passes it a blocking function or unbuffered channel, that is their issue. However, it can cause us headaches to support them.I would consider having each call that sends to the channel or calls
func()
to be in a separate short-lived goroutine, which calls fund or sends to the channel, wrapped in a timeout.Frequency of Update
My initial design is to rely on the underlying
io.CopyN()
. Whatever we use for that, we use for writing updates. However, that can overwhelm if the defaultio.Copy()
is used. If I recall correctly,io.Copy()
defaults to 32K. With a 100MB blob, that is ~3000 updates. That may or may not be good.However we control the update frequency, I think it should be byte-based, not time-based. I.e. "updates every x KB" instead of "updates every y seconds." That is more useful, and also easier to implement.
In terms of controlling the update frequency, the simplest way is
CopyOption.ProgresssFrequency uint
. If it is 0, stick to the default.An alternative is to have
CopyOption.Progress
be a struct with both the channel/func (whichever is chosen) and an update frequency property.A third method - and probably the simplest - is not to control it at all, but rather have it be part of
CopyOption.Progress
. Our callCopy()
calls that / sends to channel, and it buffers as often as it wants. This is the simplest, but is subject to making our "blocking control", i.e. goroutines, being overwhelmed.Open to ideas.
Structure of update message
The
oras.Update
should be simple and contain only 2 properties:The descriptor is important for 2 reasons:
Sample
Channel:
Func:
The text was updated successfully, but these errors were encountered: