Enhanced transfers: part 1 #1265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also remove old api functions and users, mark legacy more clearly Pare down old unit tests which used the legacy api
… implicit sharing
# Conflicts: # lfs/pointer_smudge.go # lfs/transfer_queue.go # lfs/util_test.go
# Conflicts: # tools/filetools.go
| // BatchOrLegacy calls the Batch API and falls back on the Legacy API | ||
| // This is for simplicity, legacy route is not most optimal (serial) | ||
| // TODO LEGACY API: remove when legacy API removed | ||
| func BatchOrLegacy(objects []*ObjectResource, operation string) ([]*ObjectResource, error) { |
There was a problem hiding this comment.
How about a note in the documentation that says announces we set "lfs.batch" to false locally if the batch operation is unsupported?
There was a problem hiding this comment.
I actually want us to stop doing this in 1.3 & remove the legacy API. The persistent lfs.batch=false setting is a regular source of support calls when something has gone wrong either on the server, or some step in between and then persistently the LFS client refuses to use batch any more. If we keep lfs.batch in 1.3 then I'll push for adding a big warning banner in every fetch/push/pull that tells people lfs.batch=false and suggests removing it. What we do with batch/legacy is outside the scope of this PR though.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 21, 2021
The ClearTempStorage() method of the transfer Adapter interface, while implemented for most adapters, is never actually called, and moreover is dangerous in some its current implementations if it were ever to be called. Specifically in the basic upload and download adapters and the SSH adapter, the ClearTempStorage() method relies on an internal tempDir() method to return the path to the temporary directory to be cleared, which ClearTempStorage() then removes entirely. The tempDir() methods, however, can return os.TempDir() if they are unable to create a local temporary directory within the Git LFS storage directory, and also do not cache the path to the directory they first returned. So were ClearTempStorage() ever to be called in one of these adapters, its invocation of tempDir() might just return the system temporary directory (e.g., /tmp on Unix), which it would then attempt to remove. The ClearTempStorage() method was introduced in commits f124f05 and 940a91a of PR git-lfs#1265, but has never been called by any user of the transfer adapter interface. We therefore just remove this method and its implemetations from all the current tranfer adapters.
pcal43
pushed a commit
to pcal43/git-lfs-hack
that referenced
this pull request
Jul 22, 2021
The ClearTempStorage() method of the transfer Adapter interface, while implemented for most adapters, is never actually called, and moreover is dangerous in some its current implementations if it were ever to be called. Specifically in the basic upload and download adapters and the SSH adapter, the ClearTempStorage() method relies on an internal tempDir() method to return the path to the temporary directory to be cleared, which ClearTempStorage() then removes entirely. The tempDir() methods, however, can return os.TempDir() if they are unable to create a local temporary directory within the Git LFS storage directory, and also do not cache the path to the directory they first returned. So were ClearTempStorage() ever to be called in one of these adapters, its invocation of tempDir() might just return the system temporary directory (e.g., /tmp on Unix), which it would then attempt to remove. The ClearTempStorage() method was introduced in commits f124f05 and 940a91a of PR git-lfs#1265, but has never been called by any user of the transfer adapter interface. We therefore just remove this method and its implemetations from all the current tranfer adapters.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the simpler NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the simpler NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument. Third, the TestAdapterRegButBasicOnly() function, which passes without changes, no longer fully performs the checks it was intended to make, since the NewDownloadAdapter() and NewUploadAdapter() methods now always return a non-nil value, so using a non-nil response from them to prove that the "test" adapter is found is insufficient. We therefore update the test to confirm that the returned value from these functions is a "test" adapter, as expected, and not just a "basic" one. We also replace the use of the BasicAdapterName variable with the "basic" string to align with the other tests.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() methods revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the underlying NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument. Third, although the TestAdapterRegButBasicOnly() function passes without changes, it no longer fully performs the checks it was intended to make, since the NewDownloadAdapter() and NewUploadAdapter() methods now always return a non-nil value, so using a non-nil response from them to prove that the "test" adapter was found is insufficient. We therefore update the test to confirm that the returned value from these functions is a "test" adapter, as expected, and not just a "basic" one. We also replace the use of the BasicAdapterName variable with the "basic" string to align with the other tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 1 of supporting more flexible transfer mechanisms for upload & download. This PR changes no visible behaviour or APIs but prepares the ground for features like resumable transfers and binary chunking/diffing.
The major changes are:
transferpackageTransferAdapter, and the existing path of non-resumable HTTP GET/PUT is implemented asbasicTransferAdapterTransferQueueis still used since it orchestrates the end-to-end process, which includes calling the API. However it no longer has any transfer workers, it delegates that toTransferAdapter.// TODO LEGACY API. Use of this code has been minimised so it's no longer generally mixed in even when you have batch support.git lfs fetchorgit lfs smudge. Previously there were multiple paths.DownloadCheckableand simplified things so it just uses thedryRunflag instead when probing the API for possible downloads without actually downloading. No other code used this anyway.Transferablestill exists but is now just a statement of intent in thelfspackage for the entire process of queueing through the API and eventually being transferred ifdryRun=false. It still needs to be there as an abstraction since the data comes from different places, but beyond data wrapping has no real functionality anymore barring a legacy hook which will get removed when we move to batch only. The simplerTransferstruct in thetransferpackage is used to schedule the actual transfers (doesn't shareTransferableboth to avoid import cycles and because the intent is different)As mentioned this is only an internal change right now (albeit a fairly large one) and behaviour is unaffected. The next step is to use the adapter registration system to provide additional transfer routes, negotiated between client & server through optional new fields in the API.