[Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer #12487

sunsw1994 · 2020-11-11T12:12:04Z

No description provided.

…nual convenient layer

xirzec

This looks really nice for a draft PR!

So nice, in fact, that I decided to read through and leave some comments. My apologies if you weren't ready for feedback yet. :)

xirzec · 2020-11-11T19:50:18Z

sdk/synapse/synapse-spark/package.json

+  "author": "Microsoft Corporation",
+  "description": "Azure client library for Azure Synapse Spark.",
+  "version": "1.0.0-beta.1",
+  "dependencies": {


nit: could you alpha-sort the keys in dependencies and devDependencies? #Resolved

Great recommend, will update at next commit. #Resolved

xirzec · 2020-11-11T19:52:08Z

sdk/synapse/synapse-spark/review/synapse-spark.api.md

+
+// @public
+export interface SparkClientOptions extends PipelineOptions {
+    endpoint?: string;


how is this endpoint different from the workspaceEndpoint in the constructor? #Resolved

Useless,. will removed that.

In reply to: 521600916 [](ancestors = 521600916)

xirzec · 2020-11-11T19:52:57Z

sdk/synapse/synapse-spark/review/synapse-spark.api.md

+    cancelSparkSession(sessionId: number, options?: CancelSparkSessionOptions): Promise<OperationResponse>;
+    cancelSparkStatement(sessionId: number, statementId: number, options?: CancelSparkStatementOptions): Promise<OperationResponse>;
+    createSparkBatchJob(sparkBatchJobOptions: SparkBatchJobOptions, options?: CreateSparkBatchJobOptions): Promise<CreateSparkBatchJobResponse>;
+    createSparkSeesion(sparkSessionOptions: SparkSessionOptions, options?: CreateSparkSessionOptions): Promise<CreateSparkSessionResponse>;


typo: session #Resolved

Great catch, thanks! #Resolved

xirzec · 2020-11-11T19:53:17Z

sdk/synapse/synapse-spark/review/synapse-spark.api.md

+    listSparkSessions(options?: ListSparkSessionsOptions): Promise<ListSparkSessionsResponse>;
+    listSparkStatements(sessionId: number, options?: ListSparkStatementsOptions): Promise<ListSparkStatementsResponse>;
+    resetSparkSessionTimeout(sessionId: number, options: ResetSparkSessionTimeoutOptions): Promise<OperationResponse>;
+    readonly workspaceEndpoint: string;


why make this public? #Resolved

It seems useless, nowhere need to invoke it, will remove this var at next commit.

In reply to: 521601506 [](ancestors = 521601506)

xirzec · 2020-11-11T19:56:42Z

sdk/synapse/synapse-spark/review/synapse-spark.api.md

+}
+
+// @public (undocumented)
+export class SparkClient {


a lot of these methods look like they're managing Long Running Operations. Can we use our standard LRO shape for this?

https://azure.github.io/azure-sdk/typescript_design.html#ts-lro
https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/core/core-lro

Maybe that's not the right primitive, but I'm not understanding how these operations are different right now. #Pending

Do you mean Create operation need to support LRO? I think we can stay current function, because that api has been used for a not short time at different languages, we don't get feedback need to support LRO.
Another question, can we keep this createSparkSession for now and add a new beginCreateSparkSession supoortted LRO in the future?

In reply to: 521603316 [](ancestors = 521603316)

One of the principles of the SDK is consistency, so I would hesitate to offer multiple ways to initiate / monitor / cancel a long running operation other than to use our LRO primitive.

I would think it would be simpler to have a consistent abstraction, especially since you should be able to change the internal workings of the LRO as needed. #Pending

sdk/synapse/synapse-spark/src/SparkClient.ts

xirzec · 2020-11-11T20:20:03Z

sdk/synapse/synapse-spark/test/utils/recorderUtils.ts

+export function uniqueString(): string {
+  return isPlaybackMode()
+    ? ""
+    : Math.random()


any reason not to use a guid here? #Resolved

Is it needed? I refer it from

azure-sdk-for-js/sdk/keyvault/keyvault-secrets/test/utils/recorderUtils.ts

Line 12 in 1f8a171

export function uniqueString(): string {

In reply to: 521615219 [](ancestors = 521615219)

@sadasant any thoughts? #Resolved

xirzec · 2020-11-11T20:20:26Z

sdk/synapse/synapse-spark/test/utils/testAuthentication.ts

+    env.AZURE_CLIENT_SECRET
+  );
+
+  const worksapceName = getWorkspaceName();


typo: workspaceName #Resolved

Thanks for catching it!

In reply to: 521615392 [](ancestors = 521615392)

xirzec · 2020-11-11T20:21:30Z

sdk/synapse/synapse-spark/test/utils/utils.common.ts

+import { env } from "@azure/test-utils-recorder";
+import * as assert from "assert";
+
+// Async iterator's polyfill for Node 8


does this really need to be inlined? core-asynciterator-polyfill is supposed to take care of this

https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/core/core-asynciterator-polyfill #Resolved

Will remove it.

In reply to: 521615917 [](ancestors = 521615917)

xirzec · 2020-11-11T20:24:48Z

sdk/synapse/synapse-spark/test/utils/utils.common.ts

+  return Sparkpool;
+}
+
+export async function assertThrowsAbortError(cb: () => Promise<any>): Promise<void> {


I'm assuming this helper is because chai's assert.throws doesn't work properly with async functions? #Resolved

To be honest, I am not sure for that. I just refer to this helper from

azure-sdk-for-js/sdk/keyvault/keyvault-secrets/test/utils/utils.common.ts

Line 23 in 1f8a171

export async function assertThrowsAbortError(cb: () => Promise<any>): Promise<void> {

In reply to: 521617527 [](ancestors = 521617527)

sunsw1994 · 2020-11-12T11:23:24Z

This looks really nice for a draft PR!

So nice, in fact, that I decided to read through and leave some comments. My apologies if you weren't ready for feedback yet. :)

In reply to: 528485992 [](ancestors = 528485992)

xirzec · 2020-11-17T01:31:11Z

@xirzec Thank you very much for reviewing! I have gone through your comments and fix most of these following your guidance. A question as we mention at below comment, synapse service plan to do board review by next week, we estimate there will be some changes from swagger, some common issuse caused by swagger will be fixed at that time.

Glad to hear it! You may find that not all languages have codegen that supports fancier swagger features, though we're investing in catching up.

We also want to confirm with you that we can't check in SDK before board review, right? Can code review from you and board review are in parallel?😊

You absolutely can check into the repo before the review, you just can't release a beta package.
#Resolved

sunsw1994 · 2020-11-17T13:09:25Z

@jeff Fisher Thanks for pointing that out. Do you mean we can't release a beta package as public preview after code check in?
I see there are released beta package : https://azure.github.io/azure-sdk/releases/latest/#javascript

Synapse service team side hope to check in beta.1 code and release a package (synapse spark/artifacts/accesscontrol) for preview by this month(11.30). So want to confirm with you whether there are any rick for this target from you or ts sdk side. Feel free to ping me at teams or email(alias:shangsu). Thanks a lot!

In reply to: 728627409 [](ancestors = 728627409)

xirzec · 2020-11-17T20:09:04Z

Synapse service team side hope to check in beta.1 code and release a package (synapse spark/artifacts/accesscontrol) for preview by this month(11.30). So want to confirm with you whether there are any rick for this target from you or ts sdk side. Feel free to ping me at teams or email(alias:shangsu). Thanks a lot!

My apologies @sunsw1994, I spoke without having all the context. We have a ship freeze in December, but there has been a special dispensation granted for Synapse.

@jonathandturner is going to be the main driver for this and I will work with him on unblocking you as best I can. I have a feeling that beta1 is going to be a little rough, but that's okay since breaking changes are allowed between betas. 😄 #Resolved

ramya-rao-a · 2021-01-31T07:07:10Z

Closing this PR as it has been superseded by #12713

init synapse spark track2 SDK based on autorest generated code and ma…

2a34e60

…nual convenient layer

sunsw1994 requested review from bterlson, KarishmaGhiya, mikeharder, ramya-rao-a and xirzec as code owners November 11, 2020 12:12

sunsw1994 marked this pull request as draft November 11, 2020 12:12

update dependencies

1f8a171

sunsw1994 force-pushed the Spark4Review branch from 5dd628c to 1f8a171 Compare November 11, 2020 13:17

xirzec reviewed Nov 11, 2020

View reviewed changes

sunsw1994 requested a review from idear1203 November 12, 2020 00:51

sunsw1994 mentioned this pull request Nov 12, 2020

[Synapse-Artifacts][Draft for Review]-Track2 dataplane SDK based on autorest generated code and manual convenient layer #12485

Closed

address comments

b9106df

sunsw1994 requested a review from xirzec November 16, 2020 02:28

sunsw1994 marked this pull request as ready for review November 17, 2020 12:55

sunsw1994 changed the title ~~[Synapse-Spark][Draft for Review]-Track2 spark client SDK based on autorest generated code and manual convenient layer~~ [Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer Nov 17, 2020

xirzec requested a review from sophiajt November 19, 2020 21:21

update generated code

686128e

sunsw1994 requested a review from praveenkuttappan as a code owner November 23, 2020 06:29

fix ci error

ddf4655

ramya-rao-a closed this Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer #12487

[Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer #12487

sunsw1994 commented Nov 11, 2020

xirzec left a comment

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020 •

edited

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020 •

edited

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 17, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 17, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020

sunsw1994 commented Nov 12, 2020 •

edited

Loading

xirzec commented Nov 17, 2020 •

edited by sunsw1994

Loading

sunsw1994 commented Nov 17, 2020

xirzec commented Nov 17, 2020 •

edited by sunsw1994

Loading

ramya-rao-a commented Jan 31, 2021

[Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer #12487

[Synapse-Spark]-Track2 spark client SDK based on autorest generated code and manual convenient layer #12487

Conversation

sunsw1994 commented Nov 11, 2020

xirzec left a comment

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 17, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 17, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

xirzec Nov 11, 2020 • edited by sunsw1994 Loading

Choose a reason for hiding this comment

sunsw1994 Nov 12, 2020

Choose a reason for hiding this comment

sunsw1994 commented Nov 12, 2020 • edited Loading

xirzec commented Nov 17, 2020 • edited by sunsw1994 Loading

sunsw1994 commented Nov 17, 2020

xirzec commented Nov 17, 2020 • edited by sunsw1994 Loading

ramya-rao-a commented Jan 31, 2021

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020 •

edited

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 Nov 12, 2020 •

edited

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 17, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 17, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

xirzec Nov 11, 2020 •

edited by sunsw1994

Loading

sunsw1994 commented Nov 12, 2020 •

edited

Loading

xirzec commented Nov 17, 2020 •

edited by sunsw1994

Loading

xirzec commented Nov 17, 2020 •

edited by sunsw1994

Loading