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

[MD] Support legacy client for data source #2204

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Aug 25, 2022

Signed-off-by: Su [email protected]

Description

Legacy clients are using elasticsearch.js
new clients are using openearch.js

Usage:
data_source.context.legacy.getClient(datasourceId).callAPI("ping", {params: something}, {options: something})
  • Add support for data source legacy client
    • launch another pool within the data-source service setup stage, specifically for legacy clients
    • returns a function interface LegacyAPICaller as required by elasticsearch.js clients
  • Refactor index pattern related logic to use data_source.legacy.getClient
  • Added UT

Issues Resolved

#2133
#2386

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@@ -65,7 +65,7 @@ const noop = () => undefined;
* OpenSearch JS client.
* @param options Options that affect the way we call the API and process the result.
*/
const callAPI = async (
export const callAPI = async (
Copy link
Member

Choose a reason for hiding this comment

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

do we have to export it

Copy link
Member Author

Choose a reason for hiding this comment

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

Have to export it, or otherwise copy the entire thing to datasource. Becuase we are trying to re-use it in the legacy client wrapper for datasource.

Copy link
Member Author

@zhongnansu zhongnansu Aug 30, 2022

Choose a reason for hiding this comment

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

Add more context. callAPI is not only the interface, but it wraps some validation, parsing logic for user input. To make sure the data_source.legacy.client has the exact same behavior as default legacy client, and doesn't break backwards compatibility for consumer plugins. We have to re-use it.

const callAPI = async (
client: Client,
endpoint: string,
clientParams: Record<string, any> = {},
options: LegacyCallAPIOptions = { wrap401Errors: true }
) => {
const clientPath = endpoint.split('.');
const api: any = get(client, clientPath);
if (!api) {
throw new Error(`called with an invalid endpoint: ${endpoint}`);
}
const apiContext = clientPath.length === 1 ? client : get(client, clientPath.slice(0, -1));
try {
return await new Promise((resolve, reject) => {
const request = api.call(apiContext, clientParams);

cc @noCharger

Copy link
Member Author

@zhongnansu zhongnansu Sep 26, 2022

Choose a reason for hiding this comment

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

With the new request from #2386, I think it worth to make a copy of callAPI and wire the wrap401Error there

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we can create a new callAPI for now, while exploring other options.

@zhongnansu zhongnansu marked this pull request as ready for review August 26, 2022 22:53
@zhongnansu zhongnansu requested a review from a team as a code owner August 26, 2022 22:53
@zhongnansu zhongnansu added the multiple datasource multiple datasource project label Aug 29, 2022
@zhongnansu zhongnansu changed the title support legacy client for data source Support legacy client for data source Aug 29, 2022
noCharger
noCharger previously approved these changes Aug 30, 2022
Copy link
Contributor

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

Overall looks good. Also curious if export callAPI is the best practice here.

@kristenTian
Copy link
Contributor

index pattern related migration lgtm

@zhongnansu
Copy link
Member Author

zhongnansu commented Sep 7, 2022

@zengyan-amazon Yan can you help take another look?I updated the PR based on our recent refactor and code changes

@seanneumann
Copy link
Contributor

Can I get an update? @zengyan-amazon ??

@zhongnansu
Copy link
Member Author

zhongnansu commented Sep 26, 2022

re-posted this PR to main, and include the changes requested in #2386
@zengyan-amazon @kristenTian @noCharger Please take another look

@zhongnansu zhongnansu changed the base branch from feature/multi-datasource to main September 26, 2022 21:22
@zhongnansu zhongnansu dismissed noCharger’s stale review September 26, 2022 21:22

The base branch was changed.

@zhongnansu zhongnansu force-pushed the md-legacy-client branch 2 times, most recently from 82a8d3a to 3a1ce44 Compare September 27, 2022 00:00
src/plugins/data_source/server/lib/error.ts Outdated Show resolved Hide resolved
Comment on lines 12 to 15
export interface OpenSearchClientPoolSetup {
getClientFromPool: (id: string) => Client | undefined;
addClientToPool: (endpoint: string, client: Client) => void;
getClientFromPool: (id: string) => Client | LegacyClient | undefined;
addClientToPool: (endpoint: string, client: Client | LegacyClient) => void;
}
Copy link
Member

Choose a reason for hiding this comment

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

we have this pool includes a LRU of <endpoint, root_client> . since now we support Legacy client, shall we have one pool for legacy client, and another pool for new client? otherwise, it will cause runtime error when when a use case need a legacy client while getting a new client root client from the pool, and verse versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are in fact 2 pool instances being initiated in the constructor of DataSourceService here. One only stores new client, the other one for legacy client.

Since the only different of pool impl for legacy or new client is the "value" type of the LRU, I didn't make a copy of client_pool to avoid having too much duplicate code. Instead I defined the LRU type as multiple type.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have 2 cache in the pool to avoid misuse the pool. but the current implementation makes sense to me.

zengyan-amazon
zengyan-amazon previously approved these changes Sep 28, 2022
zengyan-amazon
zengyan-amazon previously approved these changes Sep 28, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
kristenTian
kristenTian previously approved these changes Sep 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2022

Codecov Report

Merging #2204 (5435d9d) into main (0279588) will increase coverage by 0.00%.
The diff coverage is 68.42%.

@@           Coverage Diff           @@
##             main    #2204   +/-   ##
=======================================
  Coverage   66.74%   66.75%           
=======================================
  Files        3197     3200    +3     
  Lines       60828    60886   +58     
  Branches     9243     9250    +7     
=======================================
+ Hits        40601    40642   +41     
- Misses      18019    18032   +13     
- Partials     2208     2212    +4     
Impacted Files Coverage Δ
src/core/server/http/router/router.ts 74.54% <ø> (ø)
...r/index_patterns/fetcher/index_patterns_fetcher.ts 0.00% <ø> (ø)
...tcher/lib/field_capabilities/field_capabilities.ts 84.61% <ø> (ø)
...erver/index_patterns/fetcher/lib/opensearch_api.ts 100.00% <ø> (+36.36%) ⬆️
...index_patterns/fetcher/lib/resolve_time_pattern.ts 100.00% <ø> (ø)
src/plugins/data_source/server/lib/error.ts 100.00% <ø> (ø)
src/plugins/data/server/index_patterns/routes.ts 2.56% <50.00%> (+0.12%) ⬆️
...gins/data_source/server/client/configure_client.ts 80.00% <57.14%> (-4.85%) ⬇️
...ta_source/server/legacy/configure_legacy_client.ts 64.58% <64.58%> (ø)
.../plugins/data_source/server/data_source_service.ts 75.00% <70.00%> (-8.34%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ananzh ananzh merged commit 746b9df into opensearch-project:main Sep 29, 2022
@zhongnansu zhongnansu added backport 2.x v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 3, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2204-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 746b9df179f9f095befe5a2ec9df841fc287baf7
# Push it to GitHub
git push --set-upstream origin backport/backport-2204-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2204-to-2.x.

zhongnansu added a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Oct 3, 2022
* support legacy client for data source
* not wrap 401 error for data source client

Signed-off-by: Su <[email protected]>
(cherry picked from commit 746b9df)
joshuarrrr pushed a commit that referenced this pull request Oct 4, 2022
* support legacy client for data source
* not wrap 401 error for data source client

Signed-off-by: Su <[email protected]>
(cherry picked from commit 746b9df)
@ananzh ananzh added the enhancement New feature or request label Nov 5, 2022
@AMoo-Miki AMoo-Miki changed the title Support legacy client for data source [MD] Support legacy client for data source Nov 5, 2022
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
…search-project#2484)

* support legacy client for data source
* not wrap 401 error for data source client

Signed-off-by: Su <[email protected]>
(cherry picked from commit 746b9df)
pjfitzgibbons pushed a commit to pjfitzgibbons/OpenSearch-Dashboards that referenced this pull request Dec 1, 2022
…search-project#2484)

* support legacy client for data source
* not wrap 401 error for data source client

Signed-off-by: Su <[email protected]>
(cherry picked from commit 746b9df)
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
* support legacy client for data source
* not wrap 401 error for data source client

Signed-off-by: Su <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request multiple datasource multiple datasource project v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants