Prefetch: remove prefetch verb and add prefetch task to maintenance verb#172
Conversation
There was a problem hiding this comment.
I am thinking some of this code can get moved into PrefetchStep itself, but I want to wait on that until the maintenance jobs are moved out of the mount process and into the service.
kewillford
left a comment
There was a problem hiding this comment.
This looks good to me. Just one question.
There was a problem hiding this comment.
Are we going to rename Prefetch to FetchCommitsAndTrees or is it still going to be a prefetch step and could there be other things it does beside just fetching commits and trees?
There was a problem hiding this comment.
I went back and forth on a larger scale rename, here are what I see as some pros and cons of each option:
Pros of renaming
- Class name better matches what the class is doing
- Class name would better match
maintenancetask name (making it easier for new devs to find)
Cons of renaming
- Less consistent with VFS4G codebase (not sure how much we care about that)
cloneverb still using the term "prefetching", that would be inconsistent (unless we change it too)
In some places (e.g. PrefetchEndpointSuffix) I think we'll want to keep the name "Prefetch" but elsewhere (e.g. RunPrefetchStep, PrefetchStep) my leaning is to rename the classes/methods (and rename the --no-prefetch clone option).
I'm also not sure about renaming our current prefix for prefetch packs:
public const string PrefetchPackPrefix = "prefetch";
As the term "prefetch pack" is pretty well established, and I'm not sure what a better name would be 😃
Before I spend the time to make the change, @kewillford do you have a preference and @derrickstolee what are your thoughts?
There was a problem hiding this comment.
A bit unrelated, we should probably also do the rename for PostFetch. I'm thinking perhaps make this cleanup its own PR?
There was a problem hiding this comment.
A bit unrelated, we should probably also do the rename for PostFetch. I'm thinking perhaps make this cleanup its own PR?
Disregard, I just realized that's covered by #170
There was a problem hiding this comment.
In response to your cons:
- We are moving very far from VFS4G, so do not consider how close the code stays at this point.
- We should just rename what the
cloneverb is doing.
Our commits-and-trees packs will still be prefixed with "prefetch" and that is fine. It's an internal detail, not an external one.
I'm in favor of the rename.
There was a problem hiding this comment.
I would also prefer the rename so that someone doesn't come along later and add something the "prefetch" and then the code is doing more than just fetching commits and trees and becomes hard to understand what is really happening.
There was a problem hiding this comment.
Thanks for the feedback! I will push a commit to this PR that has the rename work, include a rename of the clone option
Remove the prefetch verb. Update the maintenance job to have a new "fetch-commits-and-trees" task which performs the same work that the prefetch verb used to perform. In the future, Scalar.Service will use "fetch-commits-and-trees" to run background prefetch.
- Remove prefetch (and sparse) lines from the "log" verb - Remove "Verb" from the name of prefetch related functional tests
|
@derrickstolee no worries! I'm glad your PR went it first, it makes it easier for me to rename Prefetch without causing more conflicts. |
ec89090 to
87c5f04
Compare
|
@derrickstolee @kewillford I just rebased and added a commit to rename Prefetch. I renamed everything except the following:
I'd like to have someone else take a look at the rename changes before I merge them, and so please let me know what you think 😃 |
Rename prefetch to FetchCommitsAndTrees (or fetch-commits-and-trees) throughout the codebase. The only exceptions are: - Code related to the prefetch endpoint because the endpoint is named "prefetch". - Code/file related to prefetch packs because they come from the prefetch endpoint, and "prefetch pack" is an established term for these pack files.
87c5f04 to
8ef5057
Compare
derrickstolee
left a comment
There was a problem hiding this comment.
I looked through the renames you made and think they are great!
Remove the prefetch verb. Update the maintenance job to have a new
"fetch-commits-and-trees" task which performs the same work that the
prefetch verb used to perform.
In the future, Scalar.Service will use "fetch-commits-and-trees" to
run background prefetch.
Resolves #166