Skip to content

Add support for writing resources and include more tests #4300

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
Merged
joshuali925 merged 6 commits into from
Sep 23, 2025
Merged

Add support for writing resources and include more tests #4300

joshuali925 merged 6 commits into from
Sep 23, 2025

Conversation

@lezzago
Copy link
Member

@lezzago lezzago commented Sep 15, 2025 •
edited
Loading

Description

Add support for writing resources and include more tests.
Fixed many experimental tag placements.

Related Issues

Resolves N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

@lezzago lezzago marked this pull request as draft September 16, 2025 15:41
@lezzago
Improve test coverage
232d619 
Signed-off-by: Ashish Agrawal <[email protected]>
@lezzago lezzago changed the base branch from feature/direct-query-prometheus to main September 16, 2025 21:53
@lezzago lezzago changed the base branch from main to feature/direct-query-prometheus September 16, 2025 21:53
@lezzago lezzago marked this pull request as ready for review September 16, 2025 22:33
joshuali925
joshuali925 reviewed Sep 17, 2025
View reviewed changes
...rc/main/java/org/opensearch/sql/directquery/rest/model/WriteDirectQueryResourcesRequest.java
private DirectQueryResourceType resourceType;
private String resourceName;
private String request;
private Map<String, String> RequestOptions;
Copy link
Member

@joshuali925 joshuali925 Sep 17, 2025

Choose a reason for hiding this comment

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

would the values ever need to be non-string?

Copy link
Member Author

@lezzago lezzago Sep 18, 2025

Choose a reason for hiding this comment

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

These are the request options so String should be safe here. The datasource that takes in this request to deserialize it to a non-string like an int if needed.

...ava/org/opensearch/sql/directquery/transport/model/GetDirectQueryResourcesActionRequest.java
}

public GetDirectQueryResourcesActionRequest(StreamInput in) throws IOException {
super(in);
Copy link
Member

@joshuali925 joshuali925 Sep 17, 2025

Choose a reason for hiding this comment

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

curious why remove this?

Copy link
Member Author

@lezzago lezzago Sep 19, 2025

Choose a reason for hiding this comment

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

I was having test failures because this would require the input stream to have all the required params for the parent classes like needed a field like nodeId. Below is an example failure from the test testStreamConstructorWithAllFields in GetDirectQueryResourcesActionRequestTest.

> Task :direct-query:test FAILED
GetDirectQueryResourcesActionRequestTest > testStreamConstructorWithAllFields() FAILED
    java.lang.NullPointerException: Cannot invoke "String.isEmpty()" because "nodeId" is null
        at org.opensearch.core.tasks.TaskId.readFromStream(TaskId.java:99)
        at org.opensearch.transport.TransportRequest.<init>(TransportRequest.java:74)
        at org.opensearch.action.ActionRequest.<init>(ActionRequest.java:58)
        at org.opensearch.sql.directquery.transport.model.GetDirectQueryResourcesActionRequest.<init>(GetDirectQueryResourcesActionRequest.java:29)
        at org.opensearch.sql.directquery.transport.model.GetDirectQueryResourcesActionRequestTest.testStreamConstructorWithAllFields(GetDirectQueryResourcesActionRequestTest.java:54)

@LantaoJin LantaoJin added the enhancement New feature or request label Sep 18, 2025
@lezzago
Address comments
e17e1ea 
Signed-off-by: Ashish Agrawal <[email protected]>
vamsimanohar
vamsimanohar reviewed Sep 22, 2025
View reviewed changes
...rg/opensearch/sql/directquery/transport/TransportWriteDirectQueryResourcesRequestAction.java Outdated

private final DirectQueryExecutorService directQueryExecutorService;

public static final String NAME = "indices:data/write/direct_query_resources";
Copy link
Member

@vamsimanohar vamsimanohar Sep 22, 2025

Choose a reason for hiding this comment

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

Is indices:data correct namespace? What is the rationale behind this?

public static final String NAME = "cluster:admin/opensearch/ql/datasources/read";

For example we are using the above for GetDatasourcesQueryRequest. The rationale here is that datasources are cluster level resource which admin can configure.

Copy link
Member

@vamsimanohar vamsimanohar Sep 22, 2025

Choose a reason for hiding this comment

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

[NIT] Also, we need to create an issue for making changes in security dashboards plugin to include all the new permissions.

@lezzago
Address more comments
806e090 
Signed-off-by: Ashish Agrawal <[email protected]>
vamsimanohar
vamsimanohar approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@vamsimanohar vamsimanohar left a comment

Choose a reason for hiding this comment

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

LGTM. I think we still need to think through on what to use for permissions.

cluster:admin/opensearch/direct_query/write/resources

@lezzago
Copy link
Member Author

lezzago commented Sep 23, 2025 •
edited
Loading

LGTM. I think we still need to think through on what to use for permissions.

cluster:admin/opensearch/direct_query/write/resources

Is this for more granular level permissions on what data sources the users would have access to? But I agree before going GA with the feature, we need to have this get more feedback from the community as well.

@joshuali925 joshuali925 merged commit 933c6b5 into opensearch-project:feature/direct-query-prometheus Sep 23, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshuali925 joshuali925 joshuali925 approved these changes

@vamsimanohar vamsimanohar vamsimanohar approved these changes

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

@yuancu yuancu Awaiting requested review from yuancu yuancu is a code owner

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lezzago @joshuali925 @vamsimanohar @LantaoJin