Skip to content

Conversation

@cjcenizal
Copy link
Contributor

This PR adds support for executing multiple rollup searches in a single request, which enables a dashboard to contain multiple rollup-based visualizations.

@elasticmachine
Copy link
Contributor

💔 Build Failed

}

async function serializeAllFetchParams(fetchParams, searchRequests, serializeFetchParams) {
const searcRequestsWithFetchParams = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

what reviewer let this typo pass?? 😇


for (let i = 0; i < request.payload.length; i++) {
const { index, query } = request.payload[i];
results.push(await callWithRequest('rollup.search', {
Copy link
Contributor

Choose a reason for hiding this comment

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

using await in a loop blocks the loop iteration until it resolves, right? could we change this a little to use Promise.all instead so that the calls are truly asynchronous?

        const results = [];

        for (let i = 0; i < request.payload.length; i++) {
          const { index, query } = request.payload[i];
          results.push(callWithRequest('rollup.search', {
            index,
            body: query,
          }));
        }

        reply(await Promise.all(results));

https://eslint.org/docs/rules/no-await-in-loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So funny, I went on a walk and realized the same thing, and I knew this comment was waiting for me when I came back...

@cjcenizal
Copy link
Contributor Author

@jen-huang Good catch, fixed.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@cjcenizal cjcenizal merged commit 2dc728d into elastic:feature/rollups Aug 7, 2018
@cjcenizal cjcenizal deleted the rollups/multi-search branch August 7, 2018 23:29
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants