[Step 1] use Observables on server search API by alexwizp · Pull Request #79874 · elastic/kibana

alexwizp · 2020-10-07T16:32:58Z

Summary

This PR is a first step of refactoring of server side Search API to use Observable instead of Promise.
What was done:

ISearchStrategy['search'] method was changed: context: RequestHandlerContext argument was moved to the end. I need it to unify this interface with client in future.
ISearchStrategy['search'] method now returns Observable instead of Promise
All search strategies were changed to return Observable. Temporary it was wrapped into from(new Promise()) but it will be refactored in step 2;

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

alexwizp · 2020-10-08T12:46:02Z

@elasticmachine merge upstream

elasticmachine · 2020-10-08T14:41:31Z

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp · 2020-10-09T10:53:05Z

@elasticmachine merge upstream

alexwizp · 2020-10-12T07:37:24Z

@elasticmachine merge upstream

lizozom

Looks good, added a couple comments

lizozom · 2020-10-12T13:00:25Z

src/plugins/data/server/search/es_search/es_search_strategy.ts

-      const uiSettingsClient = await context.core.uiSettings.client;
+    search: (request, options, context) =>
+      from(
+        new Promise<IEsSearchResponse>(async (resolve, reject) => {


We already have a promise returned from shimAbortPromise.

I guess you're creating this promise just as a temporary step, but I wanted to make sure.

I will do it in next step

lizozom · 2020-10-12T13:21:37Z

src/plugins/data/server/search/search_service.ts

+            search: <
+              SearchStrategyRequest extends IKibanaSearchRequest = IEsSearchRequest,
+              SearchStrategyResponse extends IKibanaSearchResponse = IEsSearchResponse
+            >(


Why did you have to get explicit here?
Doesn't the type come from SearchSourceDependencies?

you are right, it comes from types, but please see the return statement of that method.
return this.search<SearchStrategyRequest, SearchStrategyResponse>(
we need somehow to get required generic types: SearchStrategyRequest, SearchStrategyResponse

src/plugins/data/server/search/search_service.ts

lizozom · 2020-10-12T14:07:47Z

src/plugins/data/server/search/types.ts

+    options: ISearchOptions,
+    context: RequestHandlerContext
+  ) => Observable<SearchStrategyResponse>;
+  cancel?: (context: RequestHandlerContext, id: string) => Promise<void>;


Should we return an Observable here too?

lizozom · 2020-10-12T14:11:18Z

x-pack/plugins/security_solution/server/search_strategy/security_solution/index.ts

-      return queryFactory.parse(request, esSearchRes);
+      return es
+        .search({ ...request, params: dsl }, options, context)
+        .pipe(mergeMap((esSearchRes) => queryFactory.parse(request, esSearchRes)));


I think you can use map instead of mergeMap here. Am I missing something?

we should use mergeMap with promises. In case of map it returns unresolved promise
https://www.learnrxjs.io/learn-rxjs/operators/transformation/mergemap

https://stackblitz.com/edit/typescript-pnnsrq?file=index.ts&devtoolsheight=100

Ah, so it can be change to a map when we get rid of Promises here?

stratoula

KibanaApp code review mainly, I tested it locally, it seems ok to me.

alexwizp · 2020-10-13T08:37:59Z

@elasticmachine merge upstream

kibanamachine · 2020-10-13T10:20:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f8d61f6

Metrics [docs]

✅ unchanged

History

💚 Build #81103 succeeded 3ec4afd
💚 Build #81000 succeeded a0edc1c
💚 Build #80950 succeeded 87d90a0
💚 Build #80723 succeeded 8f80408
💚 Build #80700 succeeded 53fd3d5

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

XavierM

LGTM, everything is working as expected on our side.

* use Observables on server search API * fix PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…otphase-to-formlib * 'master' of github.com:elastic/kibana: (59 commits) [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard (elastic#80193) [Security Solution] Reduce initial bundle size (elastic#78992) [Security Solution][Resolver] Fix Resize node box-shadow bug (elastic#80223) Move observability content (elastic#79978) skip flaky suite (elastic#79389) removing kibana_datatable` in favor of `datatable` (elastic#75184) [ML] Fixes for anomaly swim lane (elastic#80299) [Lens] Smokescreen lens test unskip (elastic#80190) Improved AlertsClient tests structure by splitting a huge alerts_client.tests.ts file into a specific files defined by its responsibility. (elastic#80088) [APM] React key warning when opening popover with external resources (elastic#80328) [Step 1] use Observables on server search API (elastic#79874) Apply back pressure in Task Manager whenever Elasticsearch responds with a 429 (elastic#75666) [Lens] Leverage original http request error (elastic#79831) [Security Solution][Case] Improve ServiceConnectorCaseParams type (elastic#80109) [SECURITY_SOLUTION] Fix query on alert histogram (elastic#80219) [DOCS] Update ingest node pipelines doc (elastic#79187) [Ingest Manager] Split up OpenAPI spec file (elastic#80107) [SECURITY_SOLUTION][ENDPOINT] Fix label on Trusted App create name field (elastic#80001) [Ingest Manager] Fix agent policy bump revision to create only one POLICY_CHANGE action (elastic#80081) Grid layout fixes (elastic#80305) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

* use Observables on server search API * fix PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alexwizp force-pushed the be_search_1 branch 2 times, most recently from 0d38bb3 to 7560f58 Compare October 8, 2020 09:49

use Observables on server search API

53fd3d5

alexwizp force-pushed the be_search_1 branch from 7560f58 to 53fd3d5 Compare October 8, 2020 10:42

alexwizp changed the title ~~[WIP] use Observables on server search API~~ [Step 1] use Observables on server search API Oct 8, 2020

Merge branch 'master' into be_search_1

8f80408

alexwizp requested review from lizozom and lukasolson October 8, 2020 13:04

alexwizp self-assigned this Oct 8, 2020

alexwizp added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 Feature:Search Querying infrastructure in Kibana Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Oct 8, 2020

alexwizp marked this pull request as ready for review October 8, 2020 14:41

alexwizp requested a review from a team as a code owner October 8, 2020 14:41

alexwizp requested a review from a team October 8, 2020 14:41

alexwizp requested review from a team as code owners October 8, 2020 14:41

Merge branch 'master' into be_search_1

87d90a0

Merge branch 'master' into be_search_1

a0edc1c

lizozom approved these changes Oct 12, 2020

View reviewed changes

fix PR comments

3ec4afd

stratoula approved these changes Oct 13, 2020

View reviewed changes

Merge branch 'master' into be_search_1

f8d61f6

XavierM approved these changes Oct 13, 2020

View reviewed changes

alexwizp merged commit 1d1c3c7 into elastic:master Oct 13, 2020

alexwizp mentioned this pull request Oct 13, 2020

[7.x] [Step 1] use Observables on server search API (#79874) #80341

Merged

lizozom mentioned this pull request Oct 15, 2020

Use Observables on server search API #79502

Closed

alexwizp mentioned this pull request Dec 29, 2020

Search in frozen indices not included in TSVB when enabled in advanced settings #86706

Closed

Conversation

alexwizp commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

alexwizp commented Oct 8, 2020

Uh oh!

elasticmachine commented Oct 8, 2020

Uh oh!

alexwizp commented Oct 9, 2020

Uh oh!

alexwizp commented Oct 12, 2020

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

alexwizp commented Oct 13, 2020

Uh oh!

kibanamachine commented Oct 13, 2020

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

XavierM left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexwizp commented Oct 7, 2020 •

edited

Loading