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

Batch mget requests on dashboard load and cache field_stat results #10081

Merged
merged 6 commits into from
Feb 9, 2017
29 changes: 19 additions & 10 deletions src/ui/public/courier/fetch/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,32 @@ import ReqStatusProvider from './req_status';
export default function fetchService(Private, Promise) {

const requestQueue = Private(RequestQueueProvider);
const fetchThese = Private(FetchTheseProvider);

const immediatelyFetchThese = Private(FetchTheseProvider);
const callResponseHandlers = Private(CallResponseHandlersProvider);
const INCOMPLETE = Private(ReqStatusProvider).INCOMPLETE;

function fetchQueued(strategy) {
const requests = requestQueue.getStartable(strategy);
if (!requests.length) return Promise.resolve();
else return fetchThese(requests);
}
const debouncedFetchThese = _.debounce(() => {
const requests = requestQueue.get().filter(req => req.isFetchRequestedAndPending());
immediatelyFetchThese(requests);
}, {
wait: 10,
maxWait: 50
});

this.fetchQueued = fetchQueued;
const fetchTheseSoon = (requests) => {
requests.forEach(req => req._setFetchRequested());
debouncedFetchThese();
return Promise.all(requests.map(req => req.defer.promise));
};

this.fetchQueued = (strategy) => {
return fetchTheseSoon(requestQueue.getStartable(strategy));
};

function fetchASource(source, strategy) {
const defer = Promise.defer();

fetchThese([
fetchTheseSoon([
source._createRequest(defer)
]);

Expand All @@ -50,7 +59,7 @@ export default function fetchService(Private, Promise) {
* @param {array} reqs - the requests to fetch
* @async
*/
this.these = fetchThese;
this.these = fetchTheseSoon;

/**
* Send responses to a list of requests, used when requests
Expand Down
34 changes: 32 additions & 2 deletions src/ui/public/courier/fetch/request/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,42 @@ export default function AbstractReqProvider(Private, Promise) {
this.source = source;
this.defer = defer || Promise.defer();
this._whenAbortedHandlers = [];

requestQueue.push(this);
}

/**
* Called by the loopers to find requests that should be sent to the
* fetch() module. When a module is sent to fetch() it's _fetchRequested flag
* is set, and this consults that flag so requests are not send to fetch()
* multiple times.
*
* @return {Boolean}
*/
canStart() {
return Boolean(!this.stopped && !this.source._fetchDisabled);
return !this._fetchRequested && !this.stopped && !this.source._fetchDisabled;
}

/**
* Used to find requests that were previously sent to the fetch() module but
* have not been started yet, so they can be started.
*
* @return {Boolean}
*/
isFetchRequestedAndPending() {
return !!this._fetchRequested && !this.started;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the need for the double negative? Is it to avoid the undefined case? It'll still all work as expected right? Maybe just initialize this._fetchRequested to false in that case?

Just seems a little strange when the value is essentially a boolean, and the double negative requires an extra extra brain cycle or two to make the conversion. :)

Copy link
Member

Choose a reason for hiding this comment

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

Forces boolean true out of anything. I agree that it's unusual since it's part of a larger boolean statement and now I wonder if there's more to the trick than I remember or just a force of habit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, when it was just returning the value of _fetchRequested I was a little more concerned about it always being a boolean value, but now that it's returning the && calculation it's totally unnecessary.

}

/**
* Called by the fetch() module when this request has been sent to
* be fetched. At that point the request is somewhere between `ready-to-start`
* and `started`. The fetch module then waits a short period of time to
* allow requests to build up in the request queue, and then immediately
* fetches all requests that return true from `isFetchRequested()`
*
* @return {undefined}
*/
_setFetchRequested() {
this._fetchRequested = true;
}

start() {
Expand Down
8 changes: 7 additions & 1 deletion src/ui/public/courier/fetch/strategy/search.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export default function FetchStrategyForSearch(Private, Promise, timefilter, kbn
* @return {Promise} - a promise that is fulfilled by the request body
*/
reqsFetchParamsToBody: function (reqsFetchParams) {
const indexToListMapping = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


return Promise.map(reqsFetchParams, function (fetchParams) {
return Promise.resolve(fetchParams.index)
.then(function (indexList) {
Expand All @@ -23,7 +25,11 @@ export default function FetchStrategyForSearch(Private, Promise, timefilter, kbn
}

const timeBounds = timefilter.getBounds();
return indexList.toIndexList(timeBounds.min, timeBounds.max);

if (!indexToListMapping[indexList.id]) {
indexToListMapping[indexList.id] = indexList.toIndexList(timeBounds.min, timeBounds.max);
}
return indexToListMapping[indexList.id];
})
.then(function (indexList) {
let body = fetchParams.body || {};
Expand Down