Update C# samples to the latest SDKs #263

matthchr · 2019-01-14T23:25:08Z

Refactor TextSearch sample to more accurately reflect current best practices.
Use Microsoft.Azure.Batch 10.0.
Use Microsoft.Azure.Batch.Conventions.Files 3.3.0.
Use Microsoft.Azure.Batch.FileStaging 8.2.0.
Use Microsoft.Azure.Management.Batch 7.0.
Update Microsoft.IdentityModel.Clients.ActiveDirectory and Microsoft.Azure.Management.ResourceManager to the latest versions.
Clean up contributing.md.
Some minor code-formatting improvements.

matthchr · 2019-01-14T23:26:55Z

@bgklein - for some reason I can't request a review from you -- maybe because you're not an admin of the repo.

bgklein

Only a couple minor things

bgklein · 2019-01-15T02:19:23Z

CSharp/TextSearch/Common/Helpers.cs

        /// <param name="dumpStandardOutOnTaskSuccess">True to log the standard output file of the task even if it succeeded.  False to not log anything if the task succeeded.</param>
        /// <returns>The string containing the standard out of the file, or null if stdout could not be gathered.</returns>
-        public static async Task<string> CheckForTaskSuccessAsync(CloudTask boundTask, bool dumpStandardOutOnTaskSuccess)
+        public static async Task CheckForTaskSuccessAsync(CloudTask boundTask, bool dumpStandardOutOnTaskSuccess)


Shouldn't this return something?

It doesn't have to, it can just raise if the task failed.

I need to fix the comment though.

bgklein · 2019-01-15T02:21:37Z

CSharp/TextSearch/MapperTask/MapperTask.cs

+        private readonly string fileName;

        /// <summary>
        /// Constructs a mapper task object with the specified blob SAS.


Correct method header

bgklein · 2019-01-15T02:24:39Z

CSharp/TextSearch/readme.md

 ### About this sample
-This map-reduce style sample uses Azure Batch to perform parallel text processing on an input file by splitting it up into multiple sub-files and performing regular expression matching on each sub-file. The results are then rolled-up into a final report by a reduction phase where it is uploaded to Azure Storage.
+This map-reduce style sample uses Azure Batch to perform parallel text processing on an input file by splitting it up into multiple sub-files and performing regular expression matching on each sub-file.
+This sample makes use of a variety of Azure Batch features, such as auto-pool functionality, task `ResourceFiles` for moving data into VMs, task `OutputFiles` for moving data out of VMs, and task dependencies for specifying tasks relationships to one another.


Two new lines between new lines?

Got rid of the line break actually as it's not really needed. The final text reads fine as one paragraph I think.

bgklein · 2019-01-15T02:28:11Z

CSharp/Common/SampleHelpers.cs

        /// </summary>
        /// <param name="cloudStorageAccount">The cloud storage account.</param>
        /// <param name="containerName">The container name to construct a SAS for.</param>
        /// <returns>The container URL with the SAS.</returns>


Fix method header

bgklein · 2019-01-15T02:29:11Z

CSharp/Common/SampleHelpers.cs

                return containerSasUrl + "/" + blobName;
            }
-        }
+         }


Extra space?

Not sure how I managed to do that, but good catch - fixed.

matthchr · 2019-01-16T15:58:59Z

Thanks for reviewing @bgklein - I've updated according to your comments

bgklein · 2019-01-16T16:44:44Z

CI failure?

bgklein · 2019-01-16T18:38:13Z

CSharp/Common/SampleHelpers.cs

        /// Upload a file as a blob.
        /// </summary>
        /// <param name="cloudStorageAccount">The cloud storage account to upload the file to.</param>
        /// <param name="containerName">The name of the container to upload the blob to.</param>


container != containerName

Oops thought they were the same. Anyways missed this method header in my initial review

bgklein · 2019-01-16T19:47:42Z

CSharp/README.md

 ### [TextSearch](./TextSearch)
-This map-reduce style sample uses Azure Batch to perform parallel text processing on an input file by splitting it up into multiple sub-files and performing regular expression matching on each sub-file. A Job Manager task is used to orchestrate the mapper and reducer tasks with the results rolled-up into a final report. See the readme in the source directory for more information.
+This map-reduce style sample uses Azure Batch to perform parallel text processing on an input file by splitting it up into multiple sub-files and performing regular expression matching on each sub-file.
+This sample makes use of a variety of Azure Batch features, such as auto-pool functionality, task `ResourceFiles` for moving data into VMs, task `OutputFiles` for moving data out of VMs, and task dependencies for specifying tasks relationships to one another. See the readme in the source directory for more information.


is readme technically supposed to be lowercased or upper? I honestly don't know.

Like all upper? README?

I'm pretty sure we're not consistent, but I'll go fix casing to be the same as the actual file on disk (whatever case that is).

Oh, it's lowercase on disk there, so at least we're matching... I think I'll leave this as is?

Yea, just kinda curious. As long as we are consistent its fine

bgklein · 2019-01-16T19:49:54Z

CSharp/TextSearch/readme.md

-
-#### JobManager
-The job manager task submits mapper and reducer tasks and also monitors the status of those tasks.
+Uploads the files required for the text processing to Azure Storage and submits a job to the Azure Batch service that utilizes the autopool functionality.


autopool was previously auto-pool above. Make up your mind

bgklein

A couple more minor things but otherwise looks good to me

- Use OutputFiles where it makes sense. - Use TaskDependencies in TextSearch, as it makes more sense than a JobManager for that case.

- Microsoft.Azure.Batch.FileStaging and Microsoft.Azure.Batch.Conventions.Files.

matthchr added 2 commits January 11, 2019 10:19

Clean up CONTRIBUTING.md

6b1e94b

Update the C# samples to Microsoft.Azure.Batch 10.0

fe846c0

matthchr requested review from darylmsft and xingwu1 January 14, 2019 23:26

bgklein suggested changes Jan 15, 2019

View reviewed changes

matthchr force-pushed the feature/batch-sdk-10-update branch from f0aa268 to dfcf2d3 Compare January 16, 2019 15:58

bgklein reviewed Jan 16, 2019

View reviewed changes

bgklein approved these changes Jan 16, 2019

View reviewed changes

matthchr force-pushed the feature/batch-sdk-10-update branch from dfcf2d3 to 672616b Compare January 17, 2019 02:21

matthchr added 3 commits January 17, 2019 10:04

Sample refactoring and improvements

9d91c14

- Use OutputFiles where it makes sense. - Use TaskDependencies in TextSearch, as it makes more sense than a JobManager for that case.

Updated supporting assemblies

2429853

- Microsoft.Azure.Batch.FileStaging and Microsoft.Azure.Batch.Conventions.Files.

Update management sample dependencies

f2b4d49

matthchr force-pushed the feature/batch-sdk-10-update branch from 672616b to f2b4d49 Compare January 17, 2019 18:04

xingwu1 approved these changes Jan 17, 2019

View reviewed changes

matthchr merged commit 49b490e into Azure-Samples:master Jan 17, 2019

matthchr deleted the feature/batch-sdk-10-update branch January 17, 2019 21:11

Update C# samples to the latest SDKs #263

Update C# samples to the latest SDKs #263

Uh oh!

Conversation

matthchr commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthchr commented Jan 14, 2019

Uh oh!

bgklein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthchr commented Jan 16, 2019

Uh oh!

bgklein commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgklein left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthchr commented Jan 14, 2019 •

edited

Loading