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

[Main ]Create datasource API #1458

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

vmmusings
Copy link
Member

@vmmusings vmmusings commented Mar 21, 2023

Description

Earlier PR on 2.x branch: #1427
This PR covers Create datasource REST API.

Create datasource [REST API]

Screen Shot 2023-03-06 at 6 09 58 PM

Get datasource along with storage engine and other artifacts [Internal use case only]

Screen Shot 2023-03-06 at 6 13 39 PM

Issues Resolved

#1454

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.46%. Comparing base (d44cd39) to head (e12087a).
Report is 366 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1458   +/-   ##
=========================================
  Coverage     98.46%   98.46%           
- Complexity     3846     3869   +23     
=========================================
  Files           345      345           
  Lines          9553     9603   +50     
  Branches        613      616    +3     
=========================================
+ Hits           9406     9456   +50     
  Misses          142      142           
  Partials          5        5           
Flag Coverage Δ
sql-engine 98.46% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vmmusings vmmusings force-pushed the create-ds-main branch 3 times, most recently from 99e6fdf to b98edbb Compare March 21, 2023 23:24
@vmmusings
Copy link
Member Author

I have some high level questions because this PR seems have more influence than it looks:

  1. There is no issue related to this main feature. I think we should create something and put it to our 2.7 or roadmap

I will add all the required issues.

  1. The new .ql-data-sources index is shifting stateless SQL plugin to stateful? If the index is red for some reason, want to confirm if SQL plugin still work as before? At lease for query on default OpenSearch data source.

It should work for queries on default Opensearch Connector even in case of red .ql-datasources index. I will go through the code again and list if there are any scenarios where it breaks.

  1. Data source related code and dependencies keep growing, should we move all of them to a new datasource module. Because as I understand, core only depends on resolved DataSource or Table actually.

I was thinking on similar lines whether this code is really required in core or some other place. will discuss with you and try to move this.

** Captured important comments from other PR**

plugin/build.gradle Outdated Show resolved Hide resolved
@Override
public List<DataSourceMetadata> getDataSourceMetadata() {
if (!this.clusterService.state().routingTable().hasIndex(DATASOURCE_INDEX_NAME)) {
createDataSourcesIndex();
Copy link
Collaborator

@penghuo penghuo Mar 22, 2023

Choose a reason for hiding this comment

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

in which condition .ql-datasources index is created? plugin load time? client call create datasource API?

Copy link
Member Author

Choose a reason for hiding this comment

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

client call create datasource API.

Copy link
Member Author

Choose a reason for hiding this comment

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

check other plugins when they are creating an index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

High level comments,

  1. The CRUD operation on .ql-datasource could be rewrite as plan, then we can reuse existing query execution logic.
  2. Failure cases we should consider. for example, (1) if cluster write blocked. then plugin can not create/write. (2) if the data in .ql-datasource lost.

@vmmusings
Copy link
Member Author

vmmusings commented Mar 23, 2023

High level comments,

  1. The CRUD operation on .ql-datasource could be rewrite as plan, then we can reuse existing query execution logic.

Any advantage in doing so and also I need to stash current context of thread to behave like an admin and make changes to system index. (.ql-datasources)

  1. Failure cases we should consider. for example, (1) if cluster write blocked. then plugin can not create/write. (2) if the data in .ql-datasource lost.

Need to look into other plugins when cluster writes are blocked.[Mostly apis stop working until we fix it.]
we need to enable snapshots for .ql-datasources for better resillience.

Signed-off-by: vamsi-amazon <[email protected]>
@vmmusings vmmusings merged commit 12bc2b4 into opensearch-project:main Mar 27, 2023
@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-1458-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 12bc2b4b51828d2fbcdcbf7d316dd06551f5f26e
# Push it to GitHub
git push --set-upstream origin backport/backport-1458-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-1458-to-2.x.

vmmusings added a commit that referenced this pull request Mar 27, 2023
Signed-off-by: vamsi-amazon <[email protected]>
(cherry picked from commit 12bc2b4)
vmmusings added a commit to vmmusings/sql that referenced this pull request Mar 27, 2023
vmmusings added a commit to vmmusings/sql that referenced this pull request Mar 27, 2023
vmmusings added a commit that referenced this pull request Mar 28, 2023
@vmmusings vmmusings mentioned this pull request Mar 28, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Create Datasource API.
4 participants