Additional options for better reliability on faulty connections #494

Open
wants to merge 29 commits into
from

Projects

None yet

2 participants

@wyand
wyand commented Nov 22, 2013

Added options to clone, fetch, and quick-clone commands to help avoid/work around errors with faulty connections to TFS. Also useful for working around issues with TFS servers exhibiting the KB981898 bug.

clone, quick-clone, and fetch commands:
--verify-all will verify that all files downloaded correctly, and will retry any failures with a get on the individual file.
--verify-max-retries=N will limit the maximum number of retries for failed validations.

fetch command only:
--max-changesets=N will limit the number of changesets fetched, reducing the upfront delay on fetches from TFS repositories with long histories.

The definition of a catch-22:
😡 working over a crufty, unreliable VPN connection to overseas TFS
😄 git-tfs
😖 cant initialize git-tfs because even an individual changeset get can blow up.

wyand added some commits Nov 21, 2013
@wyand wyand added HashValue to IItem 5b96854
@wyand wyand add ITfsWorkspace Get(int) entry point that includes the known set of…
… entries, so that we dont need to refetch for verification when we already have the data
b84af51
@wyand wyand added TfsTreeEntryVerifier 030c6a5
@wyand wyand added --verify option to clone, fetch, quick-clone, quick-fetch 3473112
@wyand wyand changed knownTreeEntries from array to enumerable (sufficient) 2338e99
@wyand wyand updated verifier implementation and added verification to workspace p…
…ulls
c473c0b
@wyand wyand renamed verifier file 077902c
@wyand wyand implemented --verify-max-retries=n option 571ee2d
@wyand wyand documented new options 59edc93
@wyand wyand removed dead ITfsWorkspace.Get(int) method 7ce58ae
@wyand wyand added note for next release 1f3ba9c
@wyand wyand added note for next release 731523b
@wyand wyand added newline to vnext.md e35dfaa
@wyand wyand im so dumb 549aaed
@wyand wyand lazy struct -> lazy class 48148c9
@wyand wyand added GetChangesets variant with maxCount parameter (direct map to Ve…
…rsionContolServer.QueryHistory maxCount)
7a6935f
@wyand wyand added max changesets value to fetch 62d778c
@wyand wyand merged verify-all with max-count 89787d8
@wyand wyand fixed --max-changeset option cd220ea
@wyand wyand Merge branch 'max_changesets' into max_changesets_and_verify bbf116f
@wyand wyand moved changeset sort to server 94a5e8a
@wyand wyand updated trace message to include max coutn when set e26107b
@wyand wyand Merge branch 'max_changesets' into max_changesets_and_verify a20be9b
@wyand wyand added --max-changesets documentation 428eae1
@wyand wyand merged max_changesets 88e37a2
@wyand wyand fixed mocks a285f6e
@wyand wyand fixed deletes 5971e84
@wyand wyand closed this Nov 22, 2013
@wyand
wyand commented Nov 22, 2013

doh... trying to find a bug, when the problem was only empty directories on the initial TFS get.

@wyand wyand reopened this Nov 22, 2013
@wyand
wyand commented Nov 22, 2013

nope, for some reason the git repository is no longer being properly updated.

@wyand wyand closed this Nov 22, 2013
@spraints
Member

🚀 Something like this would be really amazing! Maybe take it on in smaller pieces? Lots of small branches are easier to handle all around.

Also, feel free to leave the PR open while you're working on it, just make sure to mention that you're still working on it.

@wyand
wyand commented Nov 25, 2013

My test was bad, not the results. Reopening, should be good to go.
Note that if the maximum number of retries is exhausted on a file that fails verification, the operation will continue as if the --verify-all option was not specified. If there is a good exception that will bubble up and stop things cleanly, TfsDownloadVerifier.EnsureValidItems is the place to look -- either in a caller when it returns false, or in the method instead of returning false.

@wyand wyand reopened this Nov 26, 2013
@spraints
Member
spraints commented Dec 2, 2013

❤️ I like this.

@spraints spraints commented on the diff Dec 2, 2013
doc/commands/fetch.md
@@ -22,6 +22,11 @@ The fetch command fetch all the new changesets from a TFS remote
don't need to put data back into TFS.
-u, --username=VALUE TFS username
-p, --password=VALUE TFS password
+ --verify-all Verifies all files pulled from TFS, and retries
+ the download if they are invalid
+ --verify-max-retries=VALUE If --verify-all is specified, the maximum number
+ of times that the download will be retries
+ --max-changesets=VALUE The maximum number of changesets to fetch
@spraints
spraints Dec 2, 2013 git-tfs member

What's the use-case for needing --max-changesets? I'd like to drop it.

@spraints spraints commented on an outdated diff Dec 2, 2013
docs/release-notes/vnext.md
@@ -0,0 +1,2 @@
+* added --verify-all and --verify-max-retries=N options to commands that get data from TFS
@spraints
spraints Dec 2, 2013 git-tfs member

Add (#494) to the end of this line, so we'll have a link back to this pull request.

@wyand
wyand commented Dec 2, 2013

re use case for --max-changesets=N:
If we are fetching hundreds of thousands of changesets, then the QueryHistory call itself can actually be a big download. The option allows the user to break up a very long (history wise) fetch into more manageable fetches.

@spraints
Member
spraints commented Dec 2, 2013

re use case for --max-changesets=N:
If we are fetching hundreds of thousands of changesets, then the QueryHistory call itself can actually be a big download. The option allows the user to break up a very long (history wise) fetch into more manageable fetches.

Cool. That's a pretty good idea. Could we just batch the queries internally, and keep the user from needing to care about how many changesets git-tfs is asking for at a time?

@wyand
wyand commented Dec 11, 2013

re --max-changesets:

We could batch them internally, but it doesn't appear that the current structure would allow any actual downloading to happen until the whole batch was fixed -- the idea on the switch is more of a "look, i know im not going to be able to get more than 10,000 changesets down before I need to pack up and leave, so don't bother with the rest." Again, only an issue for biiiiiig fetches.

Still, moving it to internal could help avoid connection errors in the process of fetching the changeset list; maybe try to get the whole thing, and then get it in chunks if there is a failure. I'm happy to make the change if you think that is a better interaction.

@spraints
Member

It seems like it should be relatively easy.. just some changes to GetChangesets so that it doesn't ask for the whole thing at once. There's an option for QueryHistory that gets TFS to sort the changesets right, though it may not be available in all the TFS versions.

@spraints spraints referenced this pull request Jan 20, 2014
Merged

Small improvements #530

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment