Add CopyFileAsync to ICoreFileStorageService #5581

joelverhagen · 2018-03-06T20:35:07Z

Progress on https://github.com/NuGet/Engineering/issues/1190
Progress on https://github.com/NuGet/Engineering/issues/1203

skofman1 · 2018-03-06T21:04:20Z

src/NuGetGallery/Services/FileSystemFileStorageService.cs

@@ -168,6 +169,34 @@ public Task SaveFileAsync(string folderName, string fileName, Stream packageFile
            return Task.FromResult(0);
        }

+        public Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName)


check input

skofman1 · 2018-03-06T21:06:25Z

src/NuGetGallery/Services/FileSystemService.cs

+
+        public virtual void Copy(string sourceFileName, string destFileName, bool overwrite)
+        {
+            File.Copy(sourceFileName, destFileName, overwrite);


can we make this async? https://stackoverflow.com/questions/882686/asynchronous-file-copy-move-in-c-sharp

I'd rather use the high level file system API on File. We don't use this in production so I'd rather favor small code. We can switch to async later if the performance will be helped.

Did some tests. Performance is worse.

skofman1 · 2018-03-06T21:08:53Z

src/NuGetGallery/Services/FileSystemFileStorageService.cs

+            {
+                return Convert.ToBase64String(hashAlgorithm.ComputeHash(fileStream));
+            }
+        }


What's the perf of this? Please test on files of size 250M

Perf is bound by disk. Note we don't use the FileSystemFileStorageService in production. We can optimize this later if necessary.

I added this so the method would have similar semantics to the blob storage implementation (which no-ops if the destination file is the same).

We still need to understand the perf. If the hash takes much longer then the copy itself, doesn't make sense to do it.

Sure, I'll get some numbers on disk and SSD.

You're right. File.Copy is freakishly fast and file comparison is slow.

Removed the check.

skofman1 · 2018-03-06T21:10:14Z

src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs

@@ -100,6 +104,60 @@ public CloudBlobCoreFileStorageService(ICloudBlobClient client)
            }
        }

+        public async Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName)


check input

skofman1 · 2018-03-06T21:13:56Z

src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs

@@ -16,6 +17,9 @@ namespace NuGetGallery
 {
    public class CloudBlobCoreFileStorageService : ICoreFileStorageService
    {
+        private static readonly TimeSpan MaxCopyDuration = TimeSpan.FromMinutes(10);
+        private static readonly TimeSpan CopyPollFrequency = TimeSpan.FromMilliseconds(100);


seems too small. What is this number based on?

Agree, I've seen some examples online that use 500ms.

skofman1 · 2018-03-06T21:14:36Z

src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs

@@ -16,6 +17,9 @@ namespace NuGetGallery
 {
    public class CloudBlobCoreFileStorageService : ICoreFileStorageService
    {
+        private static readonly TimeSpan MaxCopyDuration = TimeSpan.FromMinutes(10);


what is this number based on?

This is the max operation timeout for China uploads.

Comment would be good here

skofman1 · 2018-03-06T21:16:35Z

src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs

+                await Task.Delay(CopyPollFrequency);
+            }
+
+            if (destBlob.CopyState.Status == CopyStatus.Pending)


should we kill the copy operation in those cases?

I decided against this because we can still recover if the caller calls this CopyFileAsync method later. Consider if the timeout occurs, exception is thrown, and orchestrator tries again via service bus message. Things can still work out.

I would rather take this approach than aborting and trying again over and over.

(since would rather trust blob storage server side to get the job done eventually than us trying to start it every N minutes)

what if we trigger a copy operation and the previous copy is still in progress?

We won't even start. We have a precondition If-None-Match: *.

skofman1

🕐

Progress on NuGet/Engineering#1190 Progress on NuGet/Engineering#1203

joelverhagen · 2018-03-07T18:20:04Z

🔔 comments adressed.

joelverhagen requested review from agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum, skofman1 and scottbommarito March 6, 2018 20:35

joelverhagen force-pushed the jver-copyapi branch from 168c99a to c64a3c7 Compare March 6, 2018 20:45

skofman1 reviewed Mar 6, 2018

View reviewed changes

skofman1 requested changes Mar 6, 2018

View reviewed changes

Add CopyFileAsync to ICoreFileStorageService (#5581)

262741e

Progress on NuGet/Engineering#1190 Progress on NuGet/Engineering#1203

joelverhagen force-pushed the jver-copyapi branch from 110f141 to 262741e Compare March 7, 2018 18:19

skofman1 approved these changes Mar 7, 2018

View reviewed changes

joelverhagen merged commit 262741e into dev Mar 7, 2018

joelverhagen deleted the jver-copyapi branch March 7, 2018 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CopyFileAsync to ICoreFileStorageService #5581

Add CopyFileAsync to ICoreFileStorageService #5581

joelverhagen commented Mar 6, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 6, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 6, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 7, 2018

joelverhagen Mar 7, 2018

joelverhagen Mar 7, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 6, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 7, 2018

chenriksson Mar 7, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 6, 2018

joelverhagen Mar 7, 2018

joelverhagen Mar 7, 2018

skofman1 Mar 7, 2018

joelverhagen Mar 7, 2018

skofman1 left a comment

joelverhagen commented Mar 7, 2018

Add CopyFileAsync to ICoreFileStorageService #5581

Add CopyFileAsync to ICoreFileStorageService #5581

Conversation

joelverhagen commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 left a comment

Choose a reason for hiding this comment

joelverhagen commented Mar 7, 2018