SDK changes to support ADLS as option for default storage #2484

srsiva · 2016-10-20T12:00:40Z

This pull request contains the HDInsight SDK changes to support ADL as an option for default storage; for HDInsight clusters

azuresdkci · 2016-10-20T12:00:41Z

Can one of the admins verify this patch?

azurecla · 2016-10-20T12:00:51Z

Hi @srsiva, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (sisankar). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

hovsepm · 2016-10-21T00:26:12Z

@azuresdkci test this please

markcowl · 2016-10-24T18:51:33Z

@shefaliv can you take a look?

markcowl

Please reapond to comment

markcowl · 2016-10-24T18:55:38Z

src/ResourceManagement/HDInsight/HDInsight/Microsoft.Azure.Management.HDInsight.nuget.proj

@@ -2,8 +2,8 @@
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <SdkNuGetPackage Include="Microsoft.Azure.Management.HDInsight">
-      <PackageVersion>1.3.0</PackageVersion>
+      <PackageVersion>2.0.0</PackageVersion>


It doesn't look like there are breaking changes in this API, why the major version increase?

This is a breaking change for default storage. Hence the major version bump.

This isn't a breaking change until the old properties are removed. Then the version bump will be needed.

Some fields have been obsoleted in ClusterCreateParameters; making this a breaking change for all users of the SDK. Hence the major version bump.

src\resourcemanagement\hdinsight\hdinsight\customizationmodels\clustercreateparameters.cs:

[ObsoleteAttribute("This property is deprecated. Please use AzureStorageInfo with the DefaultStorageInfo parameter instead.", true)] public string DefaultStorageAccountName { get; set; } /// <summary> /// Gets or sets the StorageKey for the default Azure Storage Account. /// This account will be used for schemaless paths and the cluster will /// leverage to store some cluster level files. /// </summary> [ObsoleteAttribute("This property is deprecated. Please use AzureStorageInfo with the DefaultStorageInfo parameter instead.", true)] public string DefaultStorageAccountKey { get; set; } /// <summary> /// Gets or sets the StorageContainer for the default Azure Storage Account. /// This account will be used for schemaless paths and the cluster will /// leverage to store some cluster level files. /// </summary> [ObsoleteAttribute("This property is deprecated. Please use AzureStorageInfo with the DefaultStorageInfo parameter instead.", true)] public string DefaultStorageContainer { get; set; }

This does not make it a breaking change. Removing the fields will make it a breaking change.

@markcowl -- can you confirm if we are closed on your original comment about why a major version increase was required ?

These are throwing because you sepcified the second attribute as 'true', see:

For details on the absolete attribute, see: https://msdn.microsoft.com/en-us/library/22kk2b44(v=vs.90).aspx

If these properties no longer work then this is a breaking change. You will need to have a p;lan to mitigate this break in azure powershell if you plan to use that library there. Please acknowledge and then this issue will be resolved.

Yes ... the second attribute was specified as 'true' to force this to be a breaking change -- to make this an error (with appropriate error message to help the user to transition to the new way of specifying clusterCreateParams).
These properties no longer work .. any reference to these properties in the source code, will result in a compile error as shown in the image above.

Yes .. we already have the corresponding changes to azure powershell ready, that incorporates these (breaking) changes. I'll send out a pull request for the powershell changes as soon as this one is done.

@srsiva There will be no breaking changes in the next version of PowerShell. You must mitigate any breaks in the PowerShell API surface with code, or you will not be able to merge this library into PowerShell. There are several strategies for mtigating breaking changes. Please contact ius if you have questions abotu this. Otherwise ack and this is OK from our perspective.

@markcowl,

The breaks in Powershell API due to this change will be handled in a separate pull request. I'll send out a pull request with the corresponding changes to powershell, shortly. It is not a breaking change in as far as powershell is concerned.

Powershell seems to be taking a dependency on SDK via nuget packages (and the corresponding dll reference). Merging in this change should not affect powershell unless we publish the SDK nuget pkg and consume it in powershell code; and release a newer version of powershell.

shefaliv · 2016-10-24T19:47:45Z

src/ResourceManagement/HDInsight/HDInsight/Generated/ClusterOperations.cs

@@ -43,6 +43,8 @@ namespace Microsoft.Azure.Management.HDInsight
    /// </summary>
    internal partial class ClusterOperations : IServiceOperations<HDInsightManagementClient>, IClusterOperations
    {
+        private const string _userAgentString = "ARM SDK v2.0";


@srsive please update.

fixed - will be included in the upcoming PR.

You need to add the changes here. This PR will not be accepted without requested changes.

@markcowl , I have added the changes to this PR.

shefaliv · 2016-10-24T19:50:14Z

src/ResourceManagement/HDInsight/HDInsight.Tests/Helpers/GetClusterSpecHelpers.cs

@@ -22,6 +22,8 @@ namespace HDInsight.Tests.Helpers
 {
    public static class GetClusterSpecHelpers
    {
+        private const string ADLClusterRootPath = "/Clusters/SDK";


nit. can you remove this from here please? The values up here are just here because htey have to be set for recording. Can you please separate out the variable that's already been set?

This value is required for recording too.
The value set here is included in the CSM request that is sent out.
This value is required by the AzureDataLakeStoreInfo constructor - please see GetClusterSpecHelpers.cs:GetDefaultAzureDataLakeStoreInfo() for details.

Waiting for confirmation from @shefaliv, so that I can include the changes (if any) in the upcoming PR.

You need to add the changes here. This PR will not be accepted without requested changes.

Right but it's not a value that has to be configured when recording and can be committed. The variables up here are just hte variables that have to be manually set for recording at the time of recording.

@markcowl , there are no changes for this comment. @shefaliv to confirm.

@shefaliv please confirm if we can close out this comment.

@srsiva why are there no changes needed?

This is fixed now. I moved this variable within the test.

shefaliv · 2016-10-25T23:44:39Z

src/ResourceManagement/HDInsight/HDInsight.Tests/ScenarioTests/CreateClusterTests.cs

+        }
+
+        [Fact]
+        public void TestCreateDefaultFsAzureBlobClusterContainerNotSpecified()


should be unit test

It is not possible to convert this to a unit test.
Same reason as above.

@shefaliv , pls. confirm and close the comment.

fixed via d13fedd

shefaliv · 2016-10-25T23:44:44Z

src/ResourceManagement/HDInsight/HDInsight.Tests/ScenarioTests/CreateClusterTests.cs

+
+        [Fact]
+        public void TestCreateDefaultFsAzureBlobClusterUsingClusterParameters()
+        {


It is not possible to convert this to a unit test.
To convert to a unit test, I need to be able to setup a mock of IClusterOperations that actually executes the body of #1 below and verifies the call on #2 below.
When #1 completes executing, it would have created a ClusterCreateParametersExtended object and passed it as an argument to #2. We could then have called Mock.Verify with It.Is on #2.
Unfortunately, I dont think it is possible to setup the mock to fully execute certain methods (as though no mocking was setup) and mock the others.

public async Task CreateAsync(string resourceGroupName, string clusterName, ClusterCreateParameters clusterCreateParameters, CancellationToken cancellationToken)

public async Task CreateAsync(string resourceGroupName, string clusterName, ClusterCreateParametersExtended clusterCreateParameters, CancellationToken cancellationToken)

@shefaliv , pls. confirm and close the comment.

@srsiva You need to mock CreateWhithHttpMessagesAsync - it is possible to mock some methods and not others by using an instance of the actual client in your Mock to make the actual API call, while using a mock response for other methods.

fixed via d13fedd

markcowl · 2016-10-27T17:05:34Z

@azuresdkci add to whitelist

srsiva · 2016-11-03T10:11:23Z

Hi @markcowl.

All comments thus far have been addressed.
Could you please review and merge in the pull request ?

Thanks,
Siva

markcowl · 2016-11-03T19:20:16Z

@srsive, please see my comment, and shefali must also sign off.

shefaliv · 2016-10-26T20:37:10Z

src/ResourceManagement/HDInsight/HDInsight.Tests/Helpers/GetClusterSpecHelpers.cs

@@ -22,6 +22,8 @@ namespace HDInsight.Tests.Helpers
 {
    public static class GetClusterSpecHelpers
    {
+        private const string ADLClusterRootPath = "/Clusters/SDK";


Right but it's not a value that has to be configured when recording and can be committed. The variables up here are just hte variables that have to be manually set for recording at the time of recording.

shefaliv · 2016-10-26T20:39:52Z

src/ResourceManagement/HDInsight/HDInsight/Microsoft.Azure.Management.HDInsight.nuget.proj

@@ -2,8 +2,8 @@
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <SdkNuGetPackage Include="Microsoft.Azure.Management.HDInsight">
-      <PackageVersion>1.3.0</PackageVersion>
+      <PackageVersion>2.0.0</PackageVersion>


I'll let Matthew make this call. As far as customers are concerned, this is a breaking change because using these fields will throw an error. It's the same behavior as it would be if the fields were removed, just with a more actionable error. In my opinion, the version should be a major version bump because that will indicate to customers that they will have to change their scripts when they upgrade to this and will not be able to use their already existing scripts.

shefaliv · 2016-11-03T18:17:01Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/ClusterCreateTests.cs

+            clusterOperationsMock = new Mock<ClusterOperations>(clientMock.Object);
+
+            clientMock.SetupGet(c => c.Clusters).Returns(clusterOperationsMock.Object);
+


I sent you an email about what I expect from the unit tests. Can you please address that?

This was already fixed in 056de19.

shefaliv · 2016-11-08T00:54:27Z

src/ResourceManagement/HDInsight/HDInsight.Tests/Helpers/GetClusterSpecHelpers.cs

@@ -22,6 +22,8 @@ namespace HDInsight.Tests.Helpers
 {
    public static class GetClusterSpecHelpers
    {
+        private const string ADLClusterRootPath = "/Clusters/SDK";


@srsiva why are there no changes needed?

shefaliv · 2016-11-08T01:00:17Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/StorageAccountTests.cs

+            string mooncakeStorageNameFqdn = "testblob.blob.core.chinacloudapi.cn";
+            string existingStorageKey = "testtest==";
+            string containerName = "testcontainer";
+            string existingStorageNameFqdnUri = "wasb://" + containerName +"@" + existingStorageNameFqdn;


format string

shefaliv · 2016-11-08T01:24:19Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/StorageAccountTests.cs

+            string existingStorageKey = "testtest==";
+            string containerName = "testcontainer";
+            string existingStorageNameFqdnUri = "wasb://" + containerName +"@" + existingStorageNameFqdn;
+            string paramErrorMessage = "Input cannot be empty\r\nParameter name: ";


are there constant strings you can refer to rather than hard coding this here? Otherwise changing this error will require changes in two places.

shefaliv · 2016-11-08T01:24:28Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/StorageAccountTests.cs

+            string existingDatalakeStorageShortName = "test";
+            string existingDatalakeStorageNameFqdn = "test.azuredatalakestore.net";
+            string existingDatalakeStorageNameFqdnUri = "adl://" + existingDatalakeStorageNameFqdn;
+            string paramErrorMessage = "Input cannot be empty\r\nParameter name: ";


are there constant strings you can refer to rather than hard coding this here? Otherwise changing this error will require changes in two places.

Also below in this file

shefaliv · 2016-11-08T01:25:19Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/ClusterCreateTests.cs

+{
+    public class ClusterCreateTests
+    {
+        public ClusterCreateTests()


nit. remove.

shefaliv · 2016-11-08T01:26:22Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/ClusterCreateTests.cs

+            Assert.True(ContainerMatchesNameProvided(extendedParams, clusterName), "Container name does not match the cluster name");
+        }
+
+        private bool ContainerMatchesNameProvided(ClusterCreateParametersExtended x, string name)


you can just make this AssertContainerMatchesNameProvided and put the assert.true in here so each calling method doesn't have to

I have two different messages when the assert fails. I would rather leave it the way it is.

If you really need two different errors, then please pass that in, though in my opinion the regular assert failure is fine.

Either way, I'd rather have it be self sufficient rather than forcing the user to assert when that is the only purpose this serves.

shefaliv · 2016-11-08T01:26:35Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/ClusterCreateTests.cs

+            Assert.True(ContainerMatchesNameProvided(extendedParams, clusterName), "Container name does not match the cluster name");
+        }
+
+        private bool ContainerMatchesNameProvided(ClusterCreateParametersExtended x, string name)


better name than x please

shefaliv · 2016-11-08T19:54:52Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/ClusterCreateTests.cs

+            Assert.True(ContainerMatchesNameProvided(extendedParams, clusterName), "Container name does not match the cluster name");
+        }
+
+        private bool ContainerMatchesNameProvided(ClusterCreateParametersExtended x, string name)


If you really need two different errors, then please pass that in, though in my opinion the regular assert failure is fine.

Either way, I'd rather have it be self sufficient rather than forcing the user to assert when that is the only purpose this serves.

shefaliv · 2016-11-08T19:55:10Z

src/ResourceManagement/HDInsight/HDInsight.Tests/UnitTests/StorageAccountTests.cs

+                          );
+        }
+
+        private void testAndAssert(Func<StorageInfo> func, string msg)


where are these fixes?

markcowl · 2016-11-12T00:46:40Z

@srsiva @shefaliv Shefali needs to sign off on her review

cormacpayne · 2016-11-14T20:29:22Z

@srsiva please resolve the merge conflicts and update the PR

SDK changes to support ADLS as option for default storage

4b3d3a7

azurecla added the cla-not-required label Oct 20, 2016

markcowl requested changes Oct 24, 2016

View reviewed changes

shefaliv reviewed Oct 24, 2016

View reviewed changes

shefaliv suggested changes Oct 25, 2016

View reviewed changes

markcowl assigned srsiva and shefaliv Oct 26, 2016

srsiva added 2 commits October 27, 2016 17:23

Updated the SDK version used in the User-Agent string

2f5af9e

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net

ab48d96

Converted scenariotests into unit tests

d13fedd

Removed Moq dependency in unit tests

056de19

shefaliv suggested changes Nov 8, 2016

View reviewed changes

Minor changes to tests

7b15de8

shefaliv suggested changes Nov 8, 2016

View reviewed changes

markcowl approved these changes Nov 12, 2016

View reviewed changes

Minor fixes to SDK and tests

48ae36d

Merged from Azure:master

e95118a

shefaliv approved these changes Nov 15, 2016

View reviewed changes

markcowl merged commit c0b0d2c into Azure:master Nov 16, 2016

radumi mentioned this pull request May 23, 2024

Azure Function, either In-Process or Independent Worker, catches exception but then continues executing code after the throw reaching 200 OK. #44059

Closed

		clusterOperationsMock = new Mock<ClusterOperations>(clientMock.Object);

		clientMock.SetupGet(c => c.Clusters).Returns(clusterOperationsMock.Object);

SDK changes to support ADLS as option for default storage #2484

SDK changes to support ADLS as option for default storage #2484

Conversation

srsiva commented Oct 20, 2016

azuresdkci commented Oct 20, 2016

azurecla commented Oct 20, 2016

hovsepm commented Oct 21, 2016

markcowl commented Oct 24, 2016

markcowl left a 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

markcowl Oct 26, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcowl commented Oct 27, 2016

srsiva commented Nov 3, 2016

markcowl commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srsiva Nov 8, 2016 • edited Loading

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

markcowl commented Nov 12, 2016

cormacpayne commented Nov 14, 2016

markcowl Oct 26, 2016 •

edited

Loading

srsiva Nov 8, 2016 •

edited

Loading