From 1fc648ff373528a75977c2732dd8d3190538dd0c 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 6e9a77f848..e17df0725f 100644 --- a/src/sql/azdata.proposed.d.ts +++ b/src/sql/azdata.proposed.d.ts @@ -4475,6 +4475,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 00b9ebe63d..95b671c4ec 100644 --- a/src/sql/workbench/api/common/sqlExtHostTypes.ts +++ b/src/sql/workbench/api/common/sqlExtHostTypes.ts @@ -463,6 +463,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 c169d01143..36d589b0f7 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;