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.

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

Add SearchStrategyRegistry and defaultSearchStrategy to support existing search behavior, and integrate it with CallClient. #20497

Merged
merged 32 commits into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2bd4962
Add SearchStrategyRegistry and defaultSearchStrategy to support exist…
cjcenizal Jul 5, 2018
1c1b99a
Fix typo.
cjcenizal Jul 5, 2018
a0386ea
Move fetch param logic from CallClient into defaultSearchStrategy.
cjcenizal Jul 6, 2018
ebd3544
Move defaultSearchStrategy configuration into kibana plugin via searc…
cjcenizal Jul 6, 2018
27ba85c
Fix typo in comment.
cjcenizal Jul 6, 2018
8a7e05f
Fix bug where Dashboard Only Mode was broken.
cjcenizal Jul 6, 2018
0a8442b
Don't throw ABORTED.
cjcenizal Jul 9, 2018
9777a60
Add comments and refactor for clarity.
cjcenizal Jul 9, 2018
31925d5
Refactor strategy interface to be more flexible with the arguments it…
cjcenizal Jul 10, 2018
b74224c
Add call-out react directive.
cjcenizal Jul 10, 2018
9d02ff2
Show error in Discover if user tries to access a rollup index pattern…
cjcenizal Jul 10, 2018
d48f65b
Improve logic for resolving an early response if all searchRequests h…
cjcenizal Jul 11, 2018
bcce546
Merge branch 'master' of github.com:elastic/kibana into search-strate…
cjcenizal Jul 12, 2018
5c7666c
Fix bugs in callClient using the tests as a guide.
cjcenizal Jul 12, 2018
6b97760
Fix bug caused by typo in defaultSearchStrategy.
cjcenizal Jul 13, 2018
3964b3a
Improve appearance of Discover when an unsupported rollup index patte…
cjcenizal Jul 13, 2018
596a8c9
Remove unnecessary if.
cjcenizal Jul 13, 2018
dcddc1c
Return separate searching and abort properties from strategy.search().
cjcenizal Jul 13, 2018
00a8fe5
Simplify isViable to only expect an indexPattern.
cjcenizal Jul 13, 2018
ae033f2
Remove duplicate call to respondToSearchRequests().
cjcenizal Jul 13, 2018
5bd4b01
Send responses in original order to respondToSearchRequests.
cjcenizal Jul 13, 2018
d867a3f
Fix Jest tests.
cjcenizal Jul 13, 2018
714990c
Add tests with multiple searchStrategies and fix errors in logic.
cjcenizal Jul 13, 2018
683a2f3
Merge branch 'master' of github.com:elastic/kibana into search-strate…
cjcenizal Jul 13, 2018
5ad6a55
Rename describe block.
cjcenizal Jul 13, 2018
464b568
Rename describe block.
cjcenizal Jul 13, 2018
6d4fc57
Merge branch 'master' of github.com:elastic/kibana into search-strate…
cjcenizal Jul 16, 2018
4aba1fa
Check for default type index pattern instead of rollup type.
cjcenizal Jul 16, 2018
4c51a51
CallClient now expects search to resolve with responses array.
cjcenizal Jul 16, 2018
1c4e668
Fix tests.
cjcenizal Jul 17, 2018
ff73efb
Tweak copy.
cjcenizal Jul 17, 2018
e6379d8
Merge branch 'master' of github.com:elastic/kibana into search-strate…
cjcenizal Jul 18, 2018
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: 1 addition & 0 deletions src/core_plugins/kibana/public/kibana.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import 'uiExports/devTools';
import 'uiExports/docViews';
import 'uiExports/embeddableFactories';
import 'uiExports/inspectorViews';
import 'uiExports/search';

import 'ui/autoload/all';
import './home';
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/courier/courier.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,12 @@ uiModules.get('kibana/courier').service('courier', ($rootScope, Private) => {
/**
* Fetch the pending requests.
*/
fetch = () => {
fetch() {
fetchSoon.fetchQueued().then(() => {
// Reset the timer using the time that we get this response as the starting point.
searchPoll.resetTimer();
});
};
}
}

return new Courier();
Expand Down
80 changes: 34 additions & 46 deletions src/ui/public/courier/fetch/call_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import _ from 'lodash';

import { ErrorAllowExplicitIndexProvider } from '../../error_allow_explicit_index';
import { assignSearchRequestsToSearchStrategies } from '../search_strategy';
import { IsRequestProvider } from './is_request';
import { MergeDuplicatesRequestProvider } from './merge_duplicate_requests';
import { RequestStatus } from './req_status';
Expand All @@ -39,10 +40,14 @@ export function CallClientProvider(Private, Promise, es) {
const statuses = mergeDuplicateRequests(requests);

// get the actual list of requests that we will be fetching
let requestsToFetch = statuses.filter(isRequest);
const requestsToFetch = statuses.filter(isRequest);
let execCount = requestsToFetch.length;

if (!execCount) return Promise.resolve([]);
if (!execCount) {
return Promise.resolve([]);
}

const searchStrategiesWithRequests = assignSearchRequestsToSearchStrategies(requestsToFetch);

// resolved by respond()
let esPromise = undefined;
Expand All @@ -52,14 +57,25 @@ export function CallClientProvider(Private, Promise, es) {
// for each respond with either the response or ABORTED
const respond = function (responses) {
responses = responses || [];
return Promise.map(requests, function (request, i) {
const activeSearchRequests = searchStrategiesWithRequests.reduce((allSearchRequests, { searchRequests }) => {
return allSearchRequests.concat(searchRequests);
}, [])
.filter(request => {
// We'll use the index of the request to map it to its response. If a request has already
// failed then it won't generate a response. In this case we need to remove the request
// to maintain cardinality between the list of requests and the list of corresponding
// responses.
return request !== undefined;
});

return Promise.map(activeSearchRequests, function (request, i) {
switch (statuses[i]) {
case ABORTED:
return ABORTED;
case DUPLICATE:
return request._uniq.resp;
default:
const index = _.findIndex(requestsToFetch, request);
const index = _.findIndex(activeSearchRequests, request);
if (index < 0) {
// This means the request failed.
return ABORTED;
Expand Down Expand Up @@ -106,28 +122,27 @@ export function CallClientProvider(Private, Promise, es) {
// We're going to create a new async context here, so that the logic within it can execute
// asynchronously after we've returned a reference to defer.promise.
Promise.resolve().then(async () => {
// Flatten the searchSource within each searchRequest to get the fetch params,
// e.g. body, filters, index pattern, query.
const allFetchParams = await getAllFetchParams(requestsToFetch);

// Serialize the fetch params into a format suitable for the body of an ES query.
const serializedFetchParams = await serializeAllFetchParams(allFetchParams, requestsToFetch);

// The index of the request inside requestsToFetch determines which response is mapped to it.
// If a request won't generate a response, since it already failed, we need to remove the
// request from the requestsToFetch array so the indexes will continue to match up to the
// responses correctly.
requestsToFetch = requestsToFetch.filter(request => request !== undefined);
// Execute each request using its search strategy.
const esPromises = searchStrategiesWithRequests.map(searchStrategyWithSearchRequests => {
const { searchStrategy, searchRequests } = searchStrategyWithSearchRequests;
return searchStrategy.search({ searchRequests, es, Promise, serializeFetchParams });
});

try {
// The request was aborted while we were doing the above logic.
if (isRequestAborted) {
throw ABORTED;
}

esPromise = es.msearch({ body: serializedFetchParams });
const clientResponse = await esPromise;
await respond(clientResponse.responses);
esPromise = Promise.all(esPromises);
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to update the esPromise logic to handle the esPromises array because it needs access to the individual esPromise.abort() methods

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, would you mind trying to write a test that validates that promises from the esClient are aborted correctly when a single request in a batch is aborted? I imagine it's going to be tricky but would probably be a good idea to have given the scope of this refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll need to update the esPromise logic to handle the esPromises array because it needs access to the individual esPromise.abort() methods

Can you see any problem with just removing the code that calls esPromise.abort()? What's the point of aborting them?

const segregatedResponses = await esPromise;

// Aggregate the responses returned by all of the search strategies.
const aggregatedResponses = segregatedResponses.reduce((aggregation, responses) => {
return aggregation.concat(responses.responses);
}, []);

await respond(aggregatedResponses);
} catch(error) {
if (error === ABORTED) {
return await respond();
Expand All @@ -153,32 +168,5 @@ export function CallClientProvider(Private, Promise, es) {
});
}

function getAllFetchParams(requests) {
return Promise.map(requests, (request) => {
return Promise.try(request.getFetchParams, void 0, request)
.then((fetchParams) => {
return (request.fetchParams = fetchParams);
})
.then(value => ({ resolved: value }))
.catch(error => ({ rejected: error }));
});
}

function serializeAllFetchParams(fetchParams, requestsToFetch) {
const requestsWithFetchParams = [];

// Gather the fetch param responses from all the successful requests.
fetchParams.forEach((result, index) => {
if (result.resolved) {
requestsWithFetchParams.push(result.resolved);
} else {
const request = requestsToFetch[index];
request.handleFailure(result.rejected);
requestsToFetch[index] = undefined;
}
});
return serializeFetchParams(requestsWithFetchParams);
}

return callClient;
}
71 changes: 71 additions & 0 deletions src/ui/public/courier/search_strategy/default_search_strategy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { addSearchStrategy } from './search_strategy_registry';

function getAllFetchParams(searchRequests, Promise) {
return Promise.map(searchRequests, (searchRequest) => {
return Promise.try(searchRequest.getFetchParams, void 0, searchRequest)
.then((fetchParams) => {
return (searchRequest.fetchParams = fetchParams);
})
.then(value => ({ resolved: value }))
.catch(error => ({ rejected: error }));
});
}

function serializeAllFetchParams(fetchParams, searchRequests, serializeFetchParams) {
const requestsWithFetchParams = [];

// Gather the fetch param responses from all the successful requests.
fetchParams.forEach((result, index) => {
if (result.resolved) {
requestsWithFetchParams.push(result.resolved);
} else {
const request = searchRequests[index];
request.handleFailure(result.rejected);
searchRequests[index] = undefined;
}
});

return serializeFetchParams(requestsWithFetchParams);
}

export const defaultSearchStrategy = {
id: 'default',

search: async ({ searchRequests, es, Promise, serializeFetchParams }) => {
// Flatten the searchSource within each searchRequest to get the fetch params,
// e.g. body, filters, index pattern, query.
const allFetchParams = await getAllFetchParams(searchRequests, Promise);

// Serialize the fetch params into a format suitable for the body of an ES query.
const serializedFetchParams = await serializeAllFetchParams(allFetchParams, searchRequests, serializeFetchParams);

return es.msearch({ body: serializedFetchParams });
},

isValidForSearchRequest: searchRequest => {
// Basic index patterns don't have `type` defined.
const indexPattern = searchRequest.source.getField('index');
return indexPattern.type == null;
},
};

addSearchStrategy(defaultSearchStrategy);
23 changes: 23 additions & 0 deletions src/ui/public/courier/search_strategy/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export {
assignSearchRequestsToSearchStrategies,
addSearchStrategy,
} from './search_strategy_registry';
69 changes: 69 additions & 0 deletions src/ui/public/courier/search_strategy/search_strategy_registry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

const searchStrategies = [];

const addSearchStrategy = searchStrategy => {
searchStrategies.push(searchStrategy);
};

/**
* Build a structure like this:
*
* [{
* searchStrategy: <rollupSearchStrategy>,
* searchRequests: []<SearchRequest>,
* }, {
* searchStrategy: <defaultSearchStrategy>,
* searchRequests: []<SearchRequest>,
* }]
*
* We use an array of objects to preserve the order of the search requests, which we use to
* deterministically associate each response with the originating request.
*/
const assignSearchRequestsToSearchStrategies = searchRequests => {
const searchStrategiesWithRequests = [];
const searchStrategyById = {};

searchRequests.forEach(searchRequest => {
const matchingSearchStrategy = searchStrategies.find(searchStrategy => searchStrategy.isValidForSearchRequest(searchRequest));
const { id } = matchingSearchStrategy;
Copy link
Contributor

@jen-huang jen-huang Jul 6, 2018

Choose a reason for hiding this comment

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

getting an error for a rollup index pattern in Discover, as it doesn't match any registered search strategies:

screen shot 2018-07-06 at 3 56 25 pm

should handle cases where there is no matching search strategy. maybe we set strategy id to default?

let searchStrategyWithRequest = searchStrategyById[id];

// Create the data structure if we don't already have it.
if (!searchStrategyWithRequest) {
searchStrategyWithRequest = {
searchStrategy: matchingSearchStrategy,
searchRequests: [],
};

searchStrategyById[id] = searchStrategyWithRequest;
searchStrategiesWithRequests.push(searchStrategyWithRequest);
}

searchStrategyWithRequest.searchRequests.push(searchRequest);
});

return searchStrategiesWithRequests;
};

export {
assignSearchRequestsToSearchStrategies,
addSearchStrategy,
};
Loading