Skip to content

[Synapse] Make Synapse PowerShell consume track 2 Spark SDK #12202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 29 commits into from
Jul 1, 2020

Conversation

wonner
Copy link
Contributor

@wonner wonner commented Jun 18, 2020

Description

Make Synapse PowerShell consume track 2 Spark SDK.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@wonner wonner requested a review from idear1203 June 18, 2020 05:41
@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@VeryEarly VeryEarly self-assigned this Jun 18, 2020
@erich-wang erich-wang self-assigned this Jun 18, 2020
@idear1203
Copy link
Contributor

@VeryEarly @erich-wang We meet the following CI build error:

  Analyzer invoked with parameters: -p D:\a\1\s\artifacts/Debug -r D:\a\1\s\artifacts//StaticAnalysisResults -h -u -m
  No value provided for the --modules-to-analyze parameter. Filtering over all built modules.
  Analyzing module under D:\a\1\s\artifacts\Debug\Az.Accounts ...
  Analyzing module under D:\a\1\s\artifacts\Debug\Az.Storage ...
  Help record Set-AzDataLakeGen2ItemACLObject has no cmdlet.
  Analyzing module under D:\a\1\s\artifacts\Debug\Az.Synapse ...
  
  Unhandled Exception: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types.
  Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
  Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
  Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
     at Tools.Common.Loaders.CmdletLoader.GetModuleMetadata(String assemblyPath) in D:\a\1\s\tools\Tools.Common\Loaders\CmdletLoader.cs:line 209
     at Tools.Common.Loaders.CmdletLoader.GetModuleMetadata(String assemblyPath, List`1 commonOutputFolders) in D:\a\1\s\tools\Tools.Common\Loaders\CmdletLoader.cs:line 54
     at StaticAnalysis.HelpAnalyzer.HelpAnalyzer.AnalyzeMarkdownHelp(IEnumerable`1 scopes, String directory, ReportLogger`1 helpLogger, List`1 processedHelpFiles, String savedDirectory) in D:\a\1\s\tools\StaticAnalysis\HelpAnalyzer\HelpAnalyzer.cs:line 279
     at StaticAnalysis.HelpAnalyzer.HelpAnalyzer.Analyze(IEnumerable`1 scopes, IEnumerable`1 modulesToAnalyze) in D:\a\1\s\tools\StaticAnalysis\HelpAnalyzer\HelpAnalyzer.cs:line 111
     at StaticAnalysis.Program.Main(String[] args) in D:\a\1\s\tools\StaticAnalysis\Program.cs:line 139
D:\a\1\s\build.proj(208,5): error MSB3073: The command "dotnet D:\a\1\s\artifacts/StaticAnalysis/StaticAnalysis.Netcore.dll -p D:\a\1\s\artifacts/Debug -r D:\a\1\s\artifacts//StaticAnalysisResults -h -u -m " exited with code -532462766.
##[error]Error: The process 'C:\Program Files\dotnet\dotnet.exe' failed with exit code 1

We did some investigation locally. It seems that there are some issues when both Az.Synapse and Az.Storage exist under the artifacts\Debug folder.

If I remove the Az.Storage from artifacts\Debug folder, the command works fine:

PS> dotnet D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysis\StaticAnalysis.Netcore.dll -p D:\code\AzureSDK\azure-powershell\artifacts\Debug -r D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysisResults -h -u -m
Analyzer invoked with parameters: -p D:\code\AzureSDK\azure-powershell\artifacts\Debug -r D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysisResults -h -u -m
No value provided for the --modules-to-analyze parameter. Filtering over all built modules.
Analyzing module under D:\code\AzureSDK\azure-powershell\artifacts\Debug\Az.Accounts ...
Analyzing module under D:\code\AzureSDK\azure-powershell\artifacts\Debug\Az.Synapse ...
PS>

But the command will fail if Az.Storage exist.

PS> dotnet D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysis\StaticAnalysis.Netcore.dll -p D:\code\AzureSDK\azure-powershell\artifacts\Debug -r D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysisResults -h -u -m
Analyzer invoked with parameters: -p D:\code\AzureSDK\azure-powershell\artifacts\Debug -r D:\code\AzureSDK\azure-powershell\artifacts\StaticAnalysisResults -h -u -m
No value provided for the --modules-to-analyze parameter. Filtering over all built modules.
Analyzing module under D:\code\AzureSDK\azure-powershell\artifacts\Debug\Az.Accounts ...
Analyzing module under D:\code\AzureSDK\azure-powershell\artifacts\Debug\Az.Storage ...
Help record Set-AzDataLakeGen2ItemACLObject has no cmdlet.
Analyzing module under D:\code\AzureSDK\azure-powershell\artifacts\Debug\Az.Synapse ...

Unhandled Exception: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types.
Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
Could not load file or assembly 'Azure.Core, Version=1.2.2.0, Culture=neutral, PublicKeyToken=92742159e12e44c8'. Could not find or load a specific file. (Exception from HRESULT: 0x80131621)
   at StaticAnalysis.Program.Main(String[] args) in D:\code\AzureSDK\azure-powershell\tools\StaticAnalysis\Program.cs:line 149

Is it because the Synapse project references the Storage project? Could you please provide suggestions about how we should fix the issue? Thanks in advance.

Comment on lines 33 to 37
<Target Name="CopyFiles" AfterTargets="Build">
<Copy SourceFiles="@(PreLoadAssemblies)" DestinationFolder="$(TargetDir)\PreloadAssemblies" />
<Copy SourceFiles="@(NetCoreAssemblies)" DestinationFolder="$(TargetDir)\NetCoreAssemblies" />
</Target>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these lines

@erich-wang
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@erich-wang erich-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there's some conflict between this PR and latest master branch, please rebase this PR based on latest master.

Comment on lines 67 to 78
private IAzureEnvironment EnsureSynapseOAuthAudienceSet(IAzureEnvironment environment)
{
if (environment != null)
{
if (!environment.IsPropertySet(SynapseOAuthEndpointResourceKey))
{
environment.SetProperty(SynapseOAuthEndpointResourceKey, SynapseOAuthEndpointResourceValue);
}
}

return environment;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment related change should be in another repo https://github.com/Azure/azure-powershell-common, you may search related code for https://github.com/Azure/azure-powershell-common/blob/master/src/Authentication.Abstractions/AzureEnvironmentConstants.cs#L118..L121 as reference.
BTW, does synapse have same endpoint for different azure cloud environments? E.g. UsGov, GermanGov environments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erich-wang , we already added environment related config in azure-powershell-common: https://github.com/Azure/azure-powershell-common/blob/master/src/Authentication.Abstractions/AzureEnvironmentConstants.cs#L194-L195 . We added Synapse related endpoints as extended properties. But this comment makes sense to me. @wonner , could you please try removing this method and authenticate as below (line 27)?

            IAccessToken accessToken1 = AzureSession.Instance.AuthenticationFactory.Authenticate(
               DefaultContext.Account,
               DefaultContext.Environment,
               DefaultContext.Tenant.Id,
               null,
               ShowDialog.Never,
               null,
               AzureEnvironment.ExtendedEndpoint.AzureSynapseAnalyticsEndpointResourceId);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erich-wang for your 2nd question. Currently Synapse only have endpoints for Global cloud and China cloud. Other national clouds are not supported.

/// <summary>
/// Default resourceId for synapse OAuth tokens
/// </summary>
public const string SynapseOAuthEndpointResourceValue = "https://dev.azuresynapse.net";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need these two constants?

@idear1203
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@erich-wang
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@erich-wang erich-wang changed the base branch from master to erich/core-static-analysis June 30, 2020 09:03
@@ -12,7 +12,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.Azure.Management.Synapse" Version="0.1.0-preview.2" />
<PackageReference Include="Microsoft.Azure.Synapse" Version="0.1.0-preview" />
<PackageReference Include="Azure.Analytics.Synapse.Spark" Version="1.0.0-preview.1" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab to spaces?

idear1203
idear1203 previously approved these changes Jul 1, 2020
Copy link
Contributor

@idear1203 idear1203 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @erich-wang @VeryEarly could you please take a look at this PR again from your end?

@erich-wang erich-wang changed the base branch from erich/core-static-analysis to master July 1, 2020 07:52
@erich-wang erich-wang dismissed idear1203’s stale review July 1, 2020 07:52

The base branch was changed.

@erich-wang erich-wang merged commit 598cef5 into Azure:master Jul 1, 2020
litchiyangMSFT pushed a commit to litchiyangMSFT/azure-powershell that referenced this pull request Aug 4, 2020
)

* update NewAzureSynapseSparkPool

* fix NewAzureSynapseSparkPool

* fix NewAzureSynapseSparkPool for auto scale

* fix New-AzSynapseSparkPool for auto scale

* add New-AzSynapseSparkPool help doc

* fix New-AzSynapseSparkPool help doc

* update parameter set name

* track2 v1.0

* track2 v2.0

* track2 v2.1

* Make Synapse PowerShell consume track 2 SDK

* Make Synapse PowerShell consume track 2 SDK

* add change log

* fix proj

* Make Synapse PowerShell consume track 2 SDK

* fix AzureSessionCredential

* update changelog

* fix AzureSessionCredential

* Add required assembly Azure.Analytics.Synapse.Spark.dll to psd1

* change tab to space

Co-authored-by: Wan Yang <[email protected]>
Co-authored-by: erich-wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants