Refactor pulling #2540
Refactor pulling #2540
Conversation
| } | ||
|
|
||
| type Source interface { | ||
| Request(file string, offset int64, hash []byte, buf []byte) error |
There was a problem hiding this comment.
Needs flags opts, as I plan to use that.
|
👍 so far. |
We should still support that, but only for files? |
|
Is there a good use case for this? Has anyone ever used it in practice, now that we have the sort ordering config available? The UI for it is kind of cumbersome and you need to be fairly quick to get in there and reorder the queue so I'm skeptical it's in wide use... |
|
Well I didn't come up with the feature myself, it was requested on the tracker. Your argument about fairly fast only applies to many dmall files. Picking which VM image to pull first is perfectly viable. Is there a lot of complexity involved in this? I though the graph is only used to work out what needs doing before we can pull the file? I don't think asking to pull a specific file is essentially no extra complexity? |
|
As it works right now (in this change), we schedule all the needed items, sort them based on the selected sort criteria, then reorder based on dependencies. Changing the order manually after really also requires redoing the dependency sort, which can change the order back. For example moving a file to the top may require creating the directory it's supposed to be in, which in turn may require renaming a file that's in the way for that directory, etc recursively. To avoid completely reshuffling the list while sorting, the current sort algorithm just switches the places of two items when the dependency requires inverting the order. The alternative would be to place all the items with explicit dependencies at the head of the queue, and then do all the rest - but then we move further away from the requested sort order, potentially... All this while we're already working of the queue that we're reordering, and we need the GUI to be able to reach all the way into the changeset which is a few levels away. It's certainly not impossible, but I'm unsure if it's worth it. Part of this is to simplify and streamline the pulling process - the dependency thing is somewhat complex, but it's isolated into a single "sort()" method that's just called once at the moment. For sure someone wanted the feature, but ... we didn't have the preset "newest first", "largest first" etc options at that time either I think. |
| // that the Progresser is called appropriately before and after. | ||
| func (c *Changeset) progressAccount(fn func(protocol.FileInfo) *opError, f fileInfo) *opError { | ||
| if !f.synthetic && c.Progresser != nil { | ||
| c.Progresser.Started(f.FileInfo) |
There was a problem hiding this comment.
You could replace 3 synthetic if branches with one early one.
|
So I've read the code on a small screen, and I don't understand why it's an issue apart from code which has to reach into the queue in a changeset. I understand that moving a file to the top will break the chosen sort order, especially it has deps to satisfy, but I think this is expected, and nobody would complain about that? I guess lets get this in without that feature, but I do feel that we'll have requests for it to comeback. |
|
I think you're right. I have some stuff to explore here anyway, will look into it again and maybe not worry too much about the consequences as it's what the user asked for and we'll retry anyway when we failed (and we need to keep the retry logic, because there are still situations when we'll fail which I'll document some time...). This now passes the normal integration tests, i.e. "works" for some values of works. :) |
|
@st-jenkins retest this please |
|
@st-jenkins retest this please, once more with feeling! |
|
@st-jenkins retest this please asdkjaslkdj asldkja ldkasj dalskdj asd |
|
@st-jenkins retest this please |
|
I just checked out and checked the UI. You need to remove the "file pull order" setting from the advanced folder dialog. If this is meant by "dissallow manual reorder". |
|
I'll split this up and post some smaller PRs |
st-review
added
the
frozen-due-to-age
Current progress
Issues