Run 1 TransferQueue per uploaded ref#2806
Conversation
ttaylorr
left a comment
There was a problem hiding this comment.
Looks great; I have a few minor comments, but this looks ready to go.
tq/meter.go
Outdated
| return | ||
| } | ||
|
|
||
| m.updates <- &tasklog.Update{ |
There was a problem hiding this comment.
I think that one thing we could do to limit the number of places we do a channel write explicitly is to teach update() to take a force bool as a formal argument, and then pass that inside the body of the struct, like:
func (m *Meter) update(force bool) {
m.updates <- &tasklog.Update{Force: true, ...}
}I don't mind the duplication, but I think that channel writes in general can be fairly nuanced, so the fewer the better.
| } | ||
|
|
||
| q.meter.Finish() | ||
| q.meter.Flush() |
There was a problem hiding this comment.
I don't think that Flush() will call close(m.updates) which means that the *tasklog.Logger won't yield to future Task implementations.
There was a problem hiding this comment.
EDIT: Re-reading your PR body makes this make sense to me. Is the update channel closed elsewhere, though?
There was a problem hiding this comment.
Yup, in ReportErrors(): https://github.com/git-lfs/git-lfs/pull/2806/files#diff-3cff4ed9c5b60b5b81240c88cd74e404R263
This started as a rewrite of #2774 against new master (due to lots of conflicts with the progress meter in #2762). However, I decided to keep it scoped to just running multiple transfer queues, leaving the actual refspec-sending stuff to a follow up PR. I think that'll make this easier to review.
commands.uploadContextis an internal type, I'm trying to use exported functions incommands, outside of the actualuploadContexttype. Also, some functions were moved to theuploadContexttype, instead of merely accepting one as an argument. There's certainly more to be done with*uploadContext, but I'd rather do that in a specific refactor PR.*tq.TransferQueuenow flushes the latest update, but doesn't close the Meter's internal channel. This is so there's only 1 line in the output, instead of 1 per ref. I'm open to having a single meter line per ref, but I think that should come with the upload output changes instead.(*uploadContext) Await()has been broken up intoCollectErrors(*tq.TransferQueue)andReportErrors(). This is soCollectErrors()can be called after each uploaded ref.