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.

Sign up for GitHub

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

Jump to bottom

Catalog Enhancements #988

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Catalog Enhancements #988

merged 1 commit into from
Nov 1, 2022

Conversation

vamsimanohar
Copy link
Member

@vamsimanohar vamsimanohar commented Oct 28, 2022 •
edited
Loading

Signed-off-by: vamsi-amazon reddyvam@amazon.com

Description

  • Refactor catalog json with properties.
  • Covers Basic Authentication and AWSSigV4 Authentication.
  • Update catalog.rst with new configuration changes and support for AWSSigV4 Configuration.
  • Update catalog.rst with the feedback received.
  • Create StorageEngine using StorageEngineFactory.
  • Prometheus Connector Documentation Page.

Issues Resolved

#940

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.

@vamsimanohar vamsimanohar changed the title Included Authenticators Catalog Enhancements Oct 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022 •
edited by codecov bot
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.76%. Comparing base (e3fe086) to head (ba8fba8).
Report is 477 commits behind head on 2.x.

❗ There is a different number of reports uploaded between BASE (e3fe086) and HEAD (ba8fba8). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (e3fe086) HEAD (ba8fba8)
sql-engine 2 0
Additional details and impacted files 
@@              Coverage Diff              @@
##                2.x     #988       +/-   ##
=============================================
- Coverage     97.60%   62.76%   -34.85%     
=============================================
  Files           308       10      -298     
  Lines          7983      658     -7325     
  Branches        520      119      -401     
=============================================
- Hits           7792      413     -7379     
- Misses          190      192        +2     
- Partials          1       53       +52
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine ?

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.

@vamsimanohar vamsimanohar added enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 28, 2022
@vamsimanohar vamsimanohar force-pushed the catalog-enhancements branch 26 times, most recently from 62e24b6 to bf25d4c Compare October 31, 2022 02:04
@vamsimanohar vamsimanohar marked this pull request as ready for review October 31, 2022 02:10
@vamsimanohar vamsimanohar requested a review from a team as a code owner October 31, 2022 02:10
dai-chen
dai-chen reviewed Oct 31, 2022
View reviewed changes
docs/user/ppl/index.rst
@@ -36,6 +36,8 @@ The query start with search command and then flowing a set of command delimited

- `Catalog Settings <admin/catalog.rst>`_

- `Prometheus Connector <admin/prometheus_connector.rst>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

The connector doc seems not dependent on language. Is it better we add it to a new Connector category/folder shared by both SQL and PPL doc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

will be addressed in a new PR.

integ-test/build.gradle Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/storage/StorageEngineFactory.java Show resolved Hide resolved
plugin/src/main/java/org/opensearch/sql/plugin/catalog/CatalogServiceImpl.java Outdated Show resolved Hide resolved
prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java Show resolved Hide resolved
prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java Outdated Show resolved Hide resolved
prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusStorageFactory.java Show resolved Hide resolved
plugin/src/main/java/org/opensearch/sql/plugin/catalog/CatalogServiceImpl.java Show resolved Hide resolved
penghuo
penghuo reviewed Oct 31, 2022
View reviewed changes
plugin/src/main/java/org/opensearch/sql/plugin/catalog/CatalogServiceImpl.java
break;
return connectorTypeStorageEngineFactoryMap
.get(catalog.getConnector())
.getStorageEngine(catalog.getName(), catalog.getProperties());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may lazy eveluate the getStorageEngine when Catalog.getStorageEngine is called. The benfit is that the failure of storageEngine construction will impact requet, but not impact Plugin itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/vamsi-amazon/sql/blob/catalog-enhancements/plugin/src/main/java/org/opensearch/sql/plugin/catalog/CatalogServiceImpl.java#L82

I am catching throwable here, this will make sure plugin construction won't get impacted.
Understood the concern that customer isn't properly communicated when catalog is misconfigured.

Will try to address the lazy evaluation in the coming release.

penghuo
penghuo reviewed Oct 31, 2022
View reviewed changes
prometheus/build.gradle Outdated Show resolved Hide resolved
@vamsimanohar vamsimanohar force-pushed the catalog-enhancements branch 6 times, most recently from 2fa34ef to 35dc8f4 Compare November 1, 2022 08:57
@vamsimanohar vamsimanohar force-pushed the catalog-enhancements branch 2 times, most recently from 37efead to 4f77382 Compare November 1, 2022 09:09
penghuo
penghuo previously approved these changes Nov 1, 2022
View reviewed changes
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.

LGTM

@vamsimanohar
Included Authenticators
ba8fba8 
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
@vamsimanohar vamsimanohar merged commit c230532 into opensearch-project:2.x Nov 1, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 1, 2022
@vamsimanohar @github-actions
Included Authenticators (#988)
d8d46b9 
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
(cherry picked from commit c230532)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 1, 2022
Merged
vamsimanohar added a commit that referenced this pull request Nov 1, 2022
@opensearch-trigger-bot @vamsimanohar
Included Authenticators (#988) (#1005)
434aace 
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
Co-authored-by: vamsi-amazon <reddyvam@amazon.com>
@vamsimanohar vamsimanohar linked an issue Nov 1, 2022 that may be closed by this pull request
Support Basic and AWSSigV4 authentication for Prometheus #940
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dai-chen dai-chen dai-chen approved these changes

@joshuali925 joshuali925 joshuali925 approved these changes

@penghuo penghuo penghuo left review comments

Assignees
No one assigned
Labels
backport 2.4 enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Basic and AWSSigV4 authentication for Prometheus
5 participants
@vamsimanohar @codecov-commenter @penghuo @joshuali925 @dai-chen