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

Create datasource API #1427

Closed
wants to merge 1 commit into from

Conversation

vamsimanohar
Copy link
Member

@vamsimanohar vamsimanohar commented Mar 11, 2023

Description

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

[List any issues this PR will resolve]

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 11, 2023

Codecov Report

Merging #1427 (eb77575) into 2.x (11d0d6d) will decrease coverage by 2.17%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##                2.x    #1427      +/-   ##
============================================
- Coverage     98.38%   96.22%   -2.17%     
+ Complexity     3699     1417    -2282     
============================================
  Files           343      152     -191     
  Lines          9123     3864    -5259     
  Branches        586      277     -309     
============================================
- Hits           8976     3718    -5258     
+ Misses          142      141       -1     
  Partials          5        5              
Flag Coverage Δ
sql-engine 96.22% <100.00%> (-2.17%) ⬇️

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

Impacted Files Coverage Δ
...l/prometheus/storage/PrometheusStorageFactory.java 100.00% <100.00%> (ø)

... and 191 files with indirect coverage changes

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

@vamsimanohar vamsimanohar force-pushed the create-ds branch 2 times, most recently from 7e1ff62 to f893785 Compare March 14, 2023 18:25
@vamsimanohar vamsimanohar marked this pull request as ready for review March 14, 2023 18:46
@vamsimanohar vamsimanohar requested a review from a team as a code owner March 14, 2023 18:46
@vamsimanohar vamsimanohar force-pushed the create-ds branch 4 times, most recently from 9ba491c to 6d18aeb Compare March 16, 2023 07:58
ps48
ps48 previously approved these changes Mar 16, 2023
@dai-chen
Copy link
Collaborator

Any special reason this has to merge to 2.x first?

Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

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
  2. 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.
  3. 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.

@vamsimanohar
Copy link
Member Author

Any special reason this has to merge to 2.x first?

I need this for pen testing on 2.7 version. I will resolve conflicts with main branch and will soon raise a PR. Following main -> 2.x -> 1.x is becoming little difficult as we mostly test and verify against the next minor version in current running major.

@vamsimanohar
Copy link
Member Author

vamsimanohar commented Mar 20, 2023

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.

@vamsimanohar vamsimanohar linked an issue Mar 20, 2023 that may be closed by this pull request
@vamsimanohar vamsimanohar force-pushed the create-ds branch 2 times, most recently from a08550e to 2747293 Compare March 20, 2023 23:01
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
@penghuo
Copy link
Collaborator

penghuo commented Mar 21, 2023

Any special reason this has to merge to 2.x first?

I need this for pen testing on 2.7 version. I will resolve conflicts with main branch and will soon raise a PR. Following main -> 2.x -> 1.x is becoming little difficult as we mostly test and verify against the next minor version in current running major.

Please always merge to main first. If there is any blocker, we can discuss. For pen testing, you can use feature branch also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Create Datasource API.
5 participants