Skip to content
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.

Sign up for GitHub

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

cancellation of interpreter execution #40238

Merged
merged 11 commits into from
Jul 30, 2019
Merged

cancellation of interpreter execution #40238

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
802f608
add AbortSignal to interpreter
ppisljar Jul 3, 2019
daf72d2
adding AbortSignal to visualize_loader
ppisljar Jul 3, 2019
332dae1
adding AbortSignal to embeddables and dashboard
ppisljar Jul 3, 2019
04e1240
passing AbortSignal to courier request handler
ppisljar Jul 3, 2019
0c429c1
Remove abort signal from dashboard and move to handler, and abort fet…
lukasolson Jul 9, 2019
095860e
Remove the rest of the references to abort signal in dashboard
lukasolson Jul 9, 2019
3c7e344
Revert changes to dashboard_app
lukasolson Jul 10, 2019
4af0b64
Remove code related to canceling visualize requests and only keep stu…
lukasolson Jul 16, 2019
89ebc13
Merge branch 'master' into interpreter/canceling
lukasolson Jul 24, 2019
d8f5208
Use createError
lukasolson Jul 25, 2019
301c7f1
Merge branch 'master' of github.com:elastic/kibana into interpreter/c…
lukasolson Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/legacy/core_plugins/kibana/public/dashboard/dashboard_app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ export interface DashboardAppScope extends ng.IScope {
getEmbeddableFactory: (type: string) => EmbeddableFactory;
getDashboardState: () => DashboardStateManager;
refresh: () => void;
abortSignal?: AbortSignal;
}

const app = uiModules.get('app/dashboard', [
Expand Down
13 changes: 4 additions & 9 deletions src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,6 @@ export class DashboardAppController {
const panelActionsRegistry = Private(ContextMenuActionsRegistryProvider);
const getUnhashableStates = Private(getUnhashableStatesProvider);
const shareContextMenuExtensions = Private(ShareContextMenuExtensionsRegistryProvider);
const abortController = new AbortController();
$scope.abortSignal = abortController.signal;

// @ts-ignore This code is going away shortly.
panelActionsStore.initializeFromRegistry(panelActionsRegistry);
Expand Down Expand Up @@ -251,17 +249,15 @@ export class DashboardAppController {
!dashboardConfig.getHideWriteControls();

$scope.updateQueryAndFetch = function({ query, dateRange }) {
if (dateRange) {
timefilter.setTime(dateRange);
}

const oldQuery = $scope.model.query;
if (_.isEqual(oldQuery, query)) {
if (_.isEqual($scope.model.query, query) && _.isEqual($scope.model.timeRange, dateRange)) {
// The user can still request a reload in the query bar, even if the
// query is the same, and in that case, we have to explicitly ask for
// a reload, since no state changes will cause it.
dashboardStateManager.requestReload();
} else {
if (dateRange) {
timefilter.setTime(dateRange);
}
$scope.model.query = query;
dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters);
}
Expand Down Expand Up @@ -569,7 +565,6 @@ export class DashboardAppController {
updateSubscription.unsubscribe();
fetchSubscription.unsubscribe();
dashboardStateManager.destroy();
abortController.abort();
});

if (
Expand Down
13 changes: 5 additions & 8 deletions src/legacy/ui/public/vis/editors/default/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,18 +164,15 @@ const defaultEditor = function ($rootScope, $compile) {
}
this._loader = loader;
this._handler = this._loader.embedVisualizationWithSavedObject(visualizationEl, this.savedObj, {
uiState: uiState,
listenOnChange: false,
timeRange: timeRange,
filters: filters,
appState: appState,
uiState,
timeRange,
filters,
appState,
});
});
} else {
this._handler.update({
timeRange: timeRange,
filters: filters,
});
this._handler.update({ timeRange, filters });
}

});
Expand Down
9 changes: 7 additions & 2 deletions src/legacy/ui/public/vis/request_handlers/courier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ const CourierRequestHandlerProvider = function () {
partialRows,
metricsAtAllLevels,
inspectorAdapters,
queryFilter
queryFilter,
abortSignal
}) {

// Create a new search source that inherits the original search source
// but has the appropriate timeRange applied via a filter.
// This is a temporary solution until we properly pass down all required
Expand Down Expand Up @@ -102,6 +102,11 @@ const CourierRequestHandlerProvider = function () {
request.stats(getRequestInspectorStats(requestSearchSource));

try {
if (abortSignal) {
abortSignal.addEventListener('abort', () => {
requestSearchSource.cancelQueued();
Copy link
Member Author

Choose a reason for hiding this comment

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

what will this actually do ?

  • cancel queued (so requests that didn't start yet ?)
  • cancel current requests ?
  • on search source or on courier level ? (what happens if the request was batched with another one ?)

Copy link
Member

Choose a reason for hiding this comment

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

It aborts all incomplete requests for this searchSource object. In practice, because of Courier, requests from different searchSource objects are batched up, and cancelling one will cancel all that are bundled within the same _msearch. I don't think this is necessarily a bad thing because it will probably always be what we want. For folks that disable batching, this will only cancel the request for this specific searchSource.

});
}
const response = await requestSearchSource.fetch();

searchSource.lastQuery = queryHash;
Expand Down
28 changes: 11 additions & 17 deletions src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ interface EmbeddedVisualizeHandlerParams extends VisualizeLoaderParams {
queryFilter: any;
autoFetch?: boolean;
pipelineDataLoader?: boolean;
abortSignal?: AbortSignal;
}

const RENDER_COMPLETE_EVENT = 'render_complete';
Expand Down Expand Up @@ -94,6 +93,12 @@ export class EmbeddedVisualizeHandler {

const forceFetch = this.shouldForceNextFetch;
this.shouldForceNextFetch = false;

// Abort any in-progress requests
if (this.abortController) this.abortController.abort();
this.abortController = new AbortController();
this.dataLoaderParams.abortSignal = this.abortController.signal;

this.fetch(forceFetch).then(this.render);
}, 100);

Expand All @@ -105,6 +110,7 @@ export class EmbeddedVisualizeHandler {
private actions: any = {};
private events$: Rx.Observable<any>;
private autoFetch: boolean;
private abortController?: AbortController;

constructor(
private readonly element: HTMLElement,
Expand All @@ -123,7 +129,6 @@ export class EmbeddedVisualizeHandler {
autoFetch = true,
pipelineDataLoader = false,
Private,
abortSignal,
} = params;

this.dataLoaderParams = {
Expand All @@ -135,7 +140,6 @@ export class EmbeddedVisualizeHandler {
uiState,
aggs: vis.getAggConfig(),
forceFetch: false,
abortSignal,
};

this.pipelineDataLoader = pipelineDataLoader;
Expand Down Expand Up @@ -226,7 +230,7 @@ export class EmbeddedVisualizeHandler {
*/
public update(params: VisualizeUpdateParams = {}) {
// Apply data- attributes to the element if specified
const dataAttrs = params.dataAttrs;
const { dataAttrs, ...otherParams } = params;
if (dataAttrs) {
Object.keys(dataAttrs).forEach(key => {
if (dataAttrs[key] === null) {
Expand All @@ -238,19 +242,8 @@ export class EmbeddedVisualizeHandler {
});
}

let fetchRequired = false;
if (params.hasOwnProperty('timeRange')) {
fetchRequired = true;
this.dataLoaderParams.timeRange = params.timeRange;
}
if (params.hasOwnProperty('filters')) {
fetchRequired = true;
this.dataLoaderParams.filters = params.filters;
}
if (params.hasOwnProperty('query')) {
fetchRequired = true;
this.dataLoaderParams.query = params.query;
}
const fetchRequired = ['timeRange', 'filters', 'query'].some(key => params.hasOwnProperty(key));
this.dataLoaderParams = { ...this.dataLoaderParams, ...otherParams };
Copy link
Member

Choose a reason for hiding this comment

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

The reason this was changed is because I had originally added 'abortSignal' in here, and didn't feel like copying/pasting the exact same code 4 times made sense when we could just do it like this. Then I ended up removing 'abortSignal' from here and actually creating it in the handler. I don't mind reverting these changes if it makes people feel more comfortable :)


if (fetchRequired) {
this.fetchAndRender();
Expand All @@ -263,6 +256,7 @@ export class EmbeddedVisualizeHandler {
*/
public destroy(): void {
this.destroyed = true;
if (this.abortController) this.abortController.abort();
this.debouncedFetchAndRender.cancel();
if (this.autoFetch) {
timefilter.off('autoRefreshFetch', this.reload);
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/common/expressions/interpreter_provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export function interpreterProvider(config: any) {

// if execution was aborted return error
if (handlers.abortSignal && handlers.abortSignal.aborted) {
return createError(`abort was requested`);
return new Error(`abort was requested`);
Copy link
Contributor

@streamich streamich Jul 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe something with word expression in it, like

new Error('Expression was aborted.');

}

// Continue re-invoking chain until it's empty
Expand Down