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

[WIP] Make apps responsible for executing their own searches #20395

Closed
wants to merge 1 commit into from

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 3, 2018

This is currently a POC which demonstrates ideas from #20364 (comment). SearchSource has been refactored to return the promise which resolves its search request, allowing for the removal of a lot of courier logic.

@cjcenizal cjcenizal added the WIP Work in progress label Jul 3, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jul 3, 2018

To be honest I find this a bit hard to read, since we did all that variable renaming in this PR too. Do you think it would be possible creating a single PR (I don't even mind if tests run on this one), that just shows the actual conceptional changes you want to show, without adding a lot of renaming?

Update: I think I found all the relevant changes, and from the visualization point of view, this looks like it won't be any problems with regards to the new pipeline.

};

$scope.fetch = _.debounce(fetch, 100);
courier.setSearchCallback(fetch);
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand this will only set a single callback to be executed when the update timer fires? That won't work imho, since the <visualize> can exist several times (on a dashboard), and it could also exist besides some saved searches. Doesn't ALL those need to have their fetch triggered? So wouldn't we need a Set of listeners here instead of a single one?

@cjcenizal
Copy link
Contributor Author

@timroes I agree, this PR is too noisy to grok easily. I'll rebase this with master once #20334 is merged, which will remove all of the irrelevant changes from the diff.

… execute its search.

- Remove fetchSoon, fetchNow, callResponseHandlers, continueIncomplete, isRequest, mergeDuplicateRequests, searchRequestQueue, and courier notifier instance.
@cjcenizal cjcenizal force-pushed the make-refresh-explicit branch from 0306156 to da8dd57 Compare July 4, 2018 00:08
@cjcenizal
Copy link
Contributor Author

@timroes I cleaned up the diff.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@rashmivkulkarni
Copy link
Contributor

@cjcenizal - r u still working on it and want to keep it opened ? There hasn't been any activity here.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor Author

Closing for now. Thanks for the ping @Rasroh!

@cjcenizal cjcenizal closed this Oct 22, 2018
@cjcenizal cjcenizal deleted the make-refresh-explicit branch June 12, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants