From 37f45b10a346ba325fd8e1017e4ce1bb01730744 Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Tue, 9 Apr 2019 17:45:05 -0700 Subject: [PATCH] Mitigate (but not fully fix) Run Cell from disconnected notebook (#4960) This is a partial fix that lays groundwork for full "Prompt to connect" if a kernel needs a connection. I am waiting on Yurong's refactoring of connection handling before doing any of the prompt work. - Adds kernel metadata about whether a connection is required. - For Jupyter, only Spark kernels are listed as requiring a connection - If this is true and there's no active connection, will show notification and not call execute In the future, this path will still be used if user is prompted to connect and cancels out. The future change will be to inject a "connect" handler from notebook.component to the cell callback and use to set connection context --- extensions/notebook/src/jupyter/jupyterKernel.ts | 6 ++++++ src/sql/azdata.proposed.d.ts | 1 + src/sql/workbench/api/common/sqlExtHostTypes.ts | 1 + src/sql/workbench/api/node/extHostNotebook.ts | 3 ++- src/sql/workbench/api/node/mainThreadNotebook.ts | 4 ++++ src/sql/workbench/parts/notebook/models/cell.ts | 4 ++++ .../workbench/services/notebook/common/sessionManager.ts | 4 ++++ .../workbench/services/notebook/sql/sqlSessionManager.ts | 4 ++++ 8 files changed, 26 insertions(+), 1 deletion(-) diff --git a/extensions/notebook/src/jupyter/jupyterKernel.ts b/extensions/notebook/src/jupyter/jupyterKernel.ts index eac72dfbcb..d45f053b9c 100644 --- a/extensions/notebook/src/jupyter/jupyterKernel.ts +++ b/extensions/notebook/src/jupyter/jupyterKernel.ts @@ -63,6 +63,12 @@ export class JupyterKernel implements nb.IKernel { return true; } + public get requiresConnection(): boolean { + // TODO would be good to have a smarter way to do this. + // for now only Spark kernels need a connection + return !!(this.kernelImpl.name && this.kernelImpl.name.toLowerCase().indexOf('spark') > -1); + } + public get isReady(): boolean { return this.kernelImpl.isReady; } diff --git a/src/sql/azdata.proposed.d.ts b/src/sql/azdata.proposed.d.ts index b90a550ca6..ef1c18b2ad 100644 --- a/src/sql/azdata.proposed.d.ts +++ b/src/sql/azdata.proposed.d.ts @@ -4545,6 +4545,7 @@ declare module 'azdata' { readonly id: string; readonly name: string; readonly supportsIntellisense: boolean; + readonly requiresConnection?: boolean; /** * Test whether the kernel is ready. */ diff --git a/src/sql/workbench/api/common/sqlExtHostTypes.ts b/src/sql/workbench/api/common/sqlExtHostTypes.ts index 2ad140e4d7..3ed11ae6c8 100644 --- a/src/sql/workbench/api/common/sqlExtHostTypes.ts +++ b/src/sql/workbench/api/common/sqlExtHostTypes.ts @@ -465,6 +465,7 @@ export interface INotebookKernelDetails { readonly id: string; readonly name: string; readonly supportsIntellisense: boolean; + readonly requiresConnection: boolean; readonly info?: any; } diff --git a/src/sql/workbench/api/node/extHostNotebook.ts b/src/sql/workbench/api/node/extHostNotebook.ts index b86e9da1c4..8896644fad 100644 --- a/src/sql/workbench/api/node/extHostNotebook.ts +++ b/src/sql/workbench/api/node/extHostNotebook.ts @@ -111,7 +111,8 @@ export class ExtHostNotebook implements ExtHostNotebookShape { id: kernel.id, info: kernel.info, name: kernel.name, - supportsIntellisense: kernel.supportsIntellisense + supportsIntellisense: kernel.supportsIntellisense, + requiresConnection: kernel.requiresConnection }; return kernelDetails; } diff --git a/src/sql/workbench/api/node/mainThreadNotebook.ts b/src/sql/workbench/api/node/mainThreadNotebook.ts index 7460e81031..5e68dbe488 100644 --- a/src/sql/workbench/api/node/mainThreadNotebook.ts +++ b/src/sql/workbench/api/node/mainThreadNotebook.ts @@ -352,6 +352,10 @@ class KernelWrapper implements azdata.nb.IKernel { return this.kernelDetails.supportsIntellisense; } + get requiresConnection(): boolean { + return this.kernelDetails.requiresConnection; + } + get info(): azdata.nb.IInfoReply { return this._info; } diff --git a/src/sql/workbench/parts/notebook/models/cell.ts b/src/sql/workbench/parts/notebook/models/cell.ts index cbb7ea265b..6099494eaa 100644 --- a/src/sql/workbench/parts/notebook/models/cell.ts +++ b/src/sql/workbench/parts/notebook/models/cell.ts @@ -210,6 +210,10 @@ export class CellModel implements ICellModel { await kernel.interrupt(); } else { // TODO update source based on editor component contents + if (kernel.requiresConnection && !this.notebookModel.activeConnection) { + this.sendNotification(notificationService, Severity.Error, localize('kernelRequiresConnection', "Please select a connection to run cells for this kernel")); + return false; + } let content = this.source; if (content) { let future = await kernel.requestExecute({ diff --git a/src/sql/workbench/services/notebook/common/sessionManager.ts b/src/sql/workbench/services/notebook/common/sessionManager.ts index 8fa589f261..198762c65a 100644 --- a/src/sql/workbench/services/notebook/common/sessionManager.ts +++ b/src/sql/workbench/services/notebook/common/sessionManager.ts @@ -116,6 +116,10 @@ class EmptyKernel implements nb.IKernel { return false; } + public get requiresConnection(): boolean { + return false; + } + public get isReady(): boolean { return true; } diff --git a/src/sql/workbench/services/notebook/sql/sqlSessionManager.ts b/src/sql/workbench/services/notebook/sql/sqlSessionManager.ts index ec221a21c9..8b33954e69 100644 --- a/src/sql/workbench/services/notebook/sql/sqlSessionManager.ts +++ b/src/sql/workbench/services/notebook/sql/sqlSessionManager.ts @@ -179,6 +179,10 @@ class SqlKernel extends Disposable implements nb.IKernel { return true; } + public get requiresConnection(): boolean { + return true; + } + public get isReady(): boolean { // should we be checking on the tools service status here? return true;