[Discover] Cancel S3 Queries Using SQL Plugin #9355

sejli · 2025-02-07T22:23:22Z

Description

When ongoing S3 queries are aborted in Discover, the backend job should also be canceled/deleted. This PR uses the SQL plugin API to delete ongoing jobs to cancel them.

Issues Resolved

Screenshot

Testing the changes

Changelog

feat: Deletes S3 Jobs in Backend when Original Query is Canceled

Check List

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

Signed-off-by: Sean Li <lnse@amazon.com>

codecov · 2025-02-07T22:40:27Z

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 61.70%. Comparing base (0994dbe) to head (6b8e830).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
...nts/server/routes/data_source_connection/routes.ts	0.00%	7 Missing ⚠️
src/plugins/query_enhancements/common/utils.ts	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9355      +/-   ##
==========================================
- Coverage   61.71%   61.70%   -0.01%     
==========================================
  Files        3816     3817       +1     
  Lines       91829    91874      +45     
  Branches    14543    14555      +12     
==========================================
+ Hits        56668    56695      +27     
- Misses      31506    31522      +16     
- Partials     3655     3657       +2

Flag	Coverage Δ
Linux_1	`29.00% <ø> (+<0.01%)`	⬆️
Linux_2	`56.46% <ø> (ø)`
Linux_3	`39.19% <ø> (+<0.01%)`	⬆️
Linux_4	`28.89% <0.00%> (-0.02%)`	⬇️
Windows_1	`29.01% <ø> (+<0.01%)`	⬆️
Windows_2	`56.41% <ø> (ø)`
Windows_3	`39.20% <ø> (+0.01%)`	⬆️
Windows_4	`28.89% <0.00%> (-0.02%)`	⬇️

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.

Signed-off-by: Sean Li <lnse@amazon.com>

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js

cypress/utils/apps/query_enhancements/constants.js

kavilla · 2025-02-12T00:18:03Z

i think looks good to me. can we confirm this is wired up ? in the places we expect. or did we have fast follows to add that. as in abort query on navigation away from discover. abort query if closing the dataset selector. abort query if new query is fired (pretty sure already done)

sejli · 2025-02-14T00:39:09Z

So far, it's wired up to the point where the delete request will be sent when the query enhancement's fetch() function is aborted. So, entirely dependent on how this fetch function picks up the AbortSignal.

abort query if new query is fired

This is the main thing that this PR acheives.

abort query if closing the dataset selector.

Do we want this? If we start loading something like databases and close the dataset selector, wouldn't we want that to load in the background? I think it's intended behavior.

as in abort query on navigation away from discover

Think this will probably need a fast followup or some improvements on how I'm doing this.

joshuali925

do we also want to add a cancel button on UI?

cypress/utils/apps/query_enhancements/constants.js

joshuali925 · 2025-02-17T21:17:33Z

src/plugins/query_enhancements/common/utils.ts

+        signal,
+      })
+      .catch(async (error) => {
+        if (error.name === 'AbortError' && context.body?.pollQueryResultsParams?.queryId) {


i can't remember but does the current UI abort requests when user clicks Run query before the previous one finishes?

For example,

user starts async query

query id returned

browser sends requests to check job status

server returns job status

browser waits a few seconds if job is pending

step 3-5 repeats until query finishes

If the request in step 3 is aborted, then this would work. But if user runs another query during step 5, would this abort and delete job for the previous query?

In this scenario, yes, it will abort if another query is run in between the short polling requests.

could you explain a bit why would it abort? i thought during waiting time, the last fetch promise will be resolved and catch won't trigger?

src/plugins/query_enhancements/server/utils/plugins.ts

src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

joshuali925 · 2025-02-17T21:21:01Z

src/plugins/query_enhancements/common/utils.ts

+            });
+          } catch (cancelError) {
+            // eslint-disable-next-line no-console
+            console.error('Failed to cancel query:', cancelError);


should it throw the cancel error?

Don't think it's necessary, since we don't want this thrown error to stop any other process.

but should we let user know if it failed? For example if I accidentally ran a select * on all the data, it might be very expensive. If I started another query or clicked cancel and I don't see any warning on UI, i would assume i won't be fully charged for the previous query but in fact it might still be running

joshuali925 · 2025-02-17T21:25:23Z

src/plugins/query_enhancements/common/utils.ts

+          try {
+            await http.fetch({
+              method: 'DELETE',
+              path: API.DATA_SOURCE.ASYNC_JOBS,


should this allow each dataset type config to define the action to delete query job? there might be other type configs using pollQueryResultsParams but does not use SQL plugin async jobs

One of our followup items to discuss is how we will add the cancel button, which is why I haven't included it in this PR. However, adding a dataset type config would be a good way to do that implementation, thanks for that tip.

got it, it might be better if S3 type config provides this catch logic as an error handler

ruchidh · 2025-02-17T21:24:47Z

src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

+        });
+        return response.noContent();
+      } catch (error) {
+        const statusCode = error.statusCode === 500 ? 503 : error.statusCode || 503;


Can you describe more or may be add comment what exactly the action being taken with these error codes,

also can we write it like to make it more readable?
const statusCode = error.statusCode && error.statusCode !== 500 ? error.statusCode : 503;

This was added as a way to handle specific errors from SQL plugin. Was mimicking the other routes, which were added in this PR.

Signed-off-by: Sean Li <lnse@amazon.com>

kavilla · 2025-02-19T20:22:07Z

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js

+          cy.get(`[title="default"]`).click();
+          cy.get(`[title="http_logs"]`).click();
+          cy.getElementByTestId('datasetSelectorNext').click();
+          cy.get(`[class="euiModalHeader__title"]`).should('contain', 'Step 2: Configure data');


nit: given that this test just verify if it aborts verifyhing if selecting the title has this increases potential points of failure unrelated to this test.

in this test it would be better to just navigate to the page with a s3 query run it and then run it again focusing on the test name.

but this should be addressed later not now.

kavilla · 2025-02-19T20:23:59Z

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js

+          cy.getElementByTestId('advancedSelectorConfirmButton').click();
+
+          // Need to wait a bit for initial query to start
+          cy.wait(3000);


how come? not blocker but something we should address is avoiding abritraty wait times like a env variable that we can control globally. step 2 would be using other features that help us wait without waiting and assuming everything will happen within 3 secs

like i said tho we shouldn't address this in this pr

kavilla · 2025-02-19T20:24:24Z

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js

+          cy.getElementByTestId(`querySubmitButton`).click();
+
+          cy.wait('@cancelRequest').then((interception) => {
+            console.log('interception.request.url:', interception.request.url);


did we want to console log?

kavilla · 2025-02-19T20:26:13Z

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js

+          cy.waitForSearch();
+          cy.get(`[data-test-subj="queryResultCompleteMsg"]`).should('be.visible');
+          cy.getElementByTestId('docTable').should('be.visible');
+          cy.getElementByTestId('docTable').find('tr').should('have.length', 11);


could we potentially luck out on this being 11? for no matter what table.

would it be better if we modify the query to be something we know like SELECT * FROM foo LIMIT 10 then have it be like LIMIT 20 and then we make sure it is still 10?

joshuali925

my main concerns are #9355 (comment) and extensibility of this implementation, but i don't think they are blocking this PR

opensearch-trigger-bot · 2025-02-19T22:16:40Z

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-9355-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3c90fa38c1bf53db1858ac1f710c3413f4e5fe7a
# Push it to GitHub
git push --set-upstream origin backport/backport-9355-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

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

initial commit for canceling s3 queries

c0e2dfd

Signed-off-by: Sean Li <lnse@amazon.com>

github-actions bot added the all-star-contributor label Feb 7, 2025

Changeset file for PR opensearch-project#9355 created/updated

f015b98

sejli added discover for discover reinvent backport 2.x v2.20.0 labels Feb 7, 2025

adding integ tests

3a695da

Signed-off-by: Sean Li <lnse@amazon.com>

sejli marked this pull request as ready for review February 10, 2025 22:46

sejli requested review from joshuali925 and huyaboo as code owners February 10, 2025 22:46

kavilla reviewed Feb 12, 2025

View reviewed changes

.../core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js Outdated Show resolved Hide resolved

kavilla reviewed Feb 12, 2025

View reviewed changes

cypress/utils/apps/query_enhancements/constants.js Outdated Show resolved Hide resolved

kavilla previously approved these changes Feb 13, 2025

View reviewed changes

joshuali925 reviewed Feb 17, 2025

View reviewed changes

ruchidh reviewed Feb 17, 2025

View reviewed changes

addressing comments

6b8e830

Signed-off-by: Sean Li <lnse@amazon.com>

sejli dismissed kavilla’s stale review via 6b8e830 February 19, 2025 19:44

kavilla reviewed Feb 19, 2025

View reviewed changes

kavilla approved these changes Feb 19, 2025

View reviewed changes

joshuali925 approved these changes Feb 19, 2025

View reviewed changes

sejli merged commit 3c90fa3 into opensearch-project:main Feb 19, 2025
72 of 73 checks passed

opensearch-trigger-bot bot added the failed backport label Feb 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Cancel S3 Queries Using SQL Plugin #9355

[Discover] Cancel S3 Queries Using SQL Plugin #9355

sejli commented Feb 7, 2025

codecov bot commented Feb 7, 2025 •

edited

Loading

kavilla commented Feb 12, 2025

sejli commented Feb 14, 2025

joshuali925 left a comment

joshuali925 Feb 17, 2025

sejli Feb 19, 2025

joshuali925 Feb 19, 2025

joshuali925 Feb 17, 2025

sejli Feb 19, 2025

joshuali925 Feb 19, 2025

joshuali925 Feb 17, 2025

sejli Feb 19, 2025

joshuali925 Feb 19, 2025

ruchidh Feb 17, 2025

sejli Feb 19, 2025

kavilla Feb 19, 2025

kavilla Feb 19, 2025

kavilla Feb 19, 2025

kavilla Feb 19, 2025

joshuali925 left a comment

opensearch-trigger-bot bot commented Feb 19, 2025

[Discover] Cancel S3 Queries Using SQL Plugin #9355

[Discover] Cancel S3 Queries Using SQL Plugin #9355

Conversation

sejli commented Feb 7, 2025

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

kavilla commented Feb 12, 2025

sejli commented Feb 14, 2025

joshuali925 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuali925 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Feb 19, 2025

codecov bot commented Feb 7, 2025 •

edited

Loading