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

[Discover] Cancel S3 Queries Using SQL Plugin #9355

Merged
merged 4 commits into from
Feb 19, 2025
Merged

[Discover] Cancel S3 Queries Using SQL Plugin #9355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0e2dfd
initial commit for canceling s3 queries
sejli Feb 7, 2025
f015b98
Changeset file for PR #9355 created/updated
opensearch-changeset-bot[bot] Feb 7, 2025
3a695da
adding integ tests
sejli Feb 10, 2025
6b8e830
addressing comments
sejli Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/9355.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Deletes S3 Jobs in Backend when Original Query is Canceled ([#9355](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9355))
40 changes: 40 additions & 0 deletions ...re_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/s3_dataset.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
DS_API,
DSM_API,
S3_CLUSTER,
JOBS_API,
} from '../../../../../utils/apps/query_enhancements/constants';
import { getRandomizedWorkspaceName } from '../../../../../utils/apps/query_enhancements/shared';
import { prepareTestSuite } from '../../../../../utils/helpers';
Expand Down Expand Up @@ -90,6 +91,11 @@ const s3DatasetTestSuite = () => {
cy.deleteWorkspaceByName(workspace);
cy.visit('/app/home');
cy.osd.createInitialWorkspaceWithDataSource(S3_CLUSTER.name, workspace);
cy.navigateToWorkSpaceSpecificPage({
workspaceName: workspace,
page: 'discover',
isEnhancement: true,
});
});
afterEach(() => {
cy.deleteWorkspaceByName(workspace);
Expand Down Expand Up @@ -141,6 +147,40 @@ const s3DatasetTestSuite = () => {
cy.getElementByTestId('docTable').should('be.visible');
cy.getElementByTestId('docTable').find('tr').should('have.length', 11);
});

it('aborts and cancels previous query when new query is started', function () {
cy.getElementByTestId(`datasetSelectorButton`).click();
cy.getElementByTestId(`datasetSelectorAdvancedButton`).click();

cy.get(`[title="S3 Connections"]`).click();
cy.get(`[title="BasicS3Connection"]`).click();
cy.get(`[title="mys3"]`).click();
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');
Copy link
Member

Choose a reason for hiding this comment

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

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.


cy.getElementByTestId('advancedSelectorLanguageSelect').select('OpenSearch SQL');
cy.getElementByTestId('advancedSelectorConfirmButton').click();

// Need to wait a bit for initial query to start
cy.wait(3000);
Copy link
Member

Choose a reason for hiding this comment

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

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


cy.intercept('DELETE', `**/${JOBS_API.DELETE}*`).as('cancelRequest');
cy.getElementByTestId(`querySubmitButton`).click();

cy.wait('@cancelRequest').then((interception) => {
console.log('interception.request.url:', interception.request.url);
Copy link
Member

Choose a reason for hiding this comment

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

did we want to console log?

// Verify the request had the correct query parameters
expect(interception.request.url).to.include('queryId=');
expect(interception.request.url).to.include('id=');
});

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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

});
});
}
);
Expand Down
5 changes: 5 additions & 0 deletions cypress/utils/apps/query_enhancements/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ export const DS_API = {
};
export const DSM_API = '/internal/data-source-management/fetchDataSourceMetaData';

export const BASE_QUERY_ENHANCEMENTS_API = '/api/enhancements';
export const JOBS_API = {
DELETE: `${BASE_QUERY_ENHANCEMENTS_API}/jobs`,
};

export const INDEX_WITH_TIME_1 = 'data_logs_small_time_1';
export const INDEX_WITHOUT_TIME_1 = 'data_logs_small_no_time_1';
export const INDEX_WITH_TIME_2 = 'data_logs_small_time_2';
Expand Down
34 changes: 28 additions & 6 deletions src/plugins/query_enhancements/common/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
QueryStatusConfig,
QueryStatusOptions,
} from './types';
import { API } from './constants';

export const formatDate = (dateString: string) => {
const date = new Date(dateString);
Expand Down Expand Up @@ -57,13 +58,34 @@
pollQueryResultsParams: context.body?.pollQueryResultsParams,
timeRange: context.body?.timeRange,
});

return from(
http.fetch({
method: 'POST',
path,
body,
signal,
})
http
.fetch({
method: 'POST',
path,
body,
signal,
})
.catch(async (error) => {
if (error.name === 'AbortError' && context.body?.pollQueryResultsParams?.queryId) {
Copy link
Member

Choose a reason for hiding this comment

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

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

For example,

  1. user starts async query
  2. query id returned
  3. browser sends requests to check job status
  4. server returns job status
  5. browser waits a few seconds if job is pending
  6. 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

// Cancel job
try {
await http.fetch({

Check warning on line 74 in src/plugins/query_enhancements/common/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/query_enhancements/common/utils.ts#L73-L74 

Added lines #L73 - L74 were not covered by tests
method: 'DELETE',
path: API.DATA_SOURCE.ASYNC_JOBS,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

query: {
id: query.dataset?.dataSource?.id,
queryId: context.body?.pollQueryResultsParams.queryId,
},
});
} catch (cancelError) {
// eslint-disable-next-line no-console
console.error('Failed to cancel query:', cancelError);

Check warning on line 84 in src/plugins/query_enhancements/common/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/query_enhancements/common/utils.ts#L84 

Added line #L84 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

should it throw the cancel error?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}
throw error;

Check warning on line 86 in src/plugins/query_enhancements/common/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/query_enhancements/common/utils.ts#L86 

Added line #L86 was not covered by tests
}
})
);
};

Expand Down
27 changes: 27 additions & 0 deletions src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,31 @@
}
}
);

router.delete(

Check warning on line 103 in src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L103 was not covered by tests
{
path: API.DATA_SOURCE.ASYNC_JOBS,
validate: {
query: schema.object({
id: schema.string(),
queryId: schema.nullable(schema.string()),
}),
},
},
async (context, request, response) => {
try {

Check warning on line 114 in src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L114 was not covered by tests
const client = request.query.id
? context.dataSource.opensearch.legacy.getClient(request.query.id).callAPI
: defaultClient.asScoped(request).callAsCurrentUser;

await client('enhancements.deleteJob', {

Check warning on line 119 in src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L119 was not covered by tests
queryId: request.query.queryId,
});
return response.noContent();

Check warning on line 122 in src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L122 was not covered by tests
} catch (error) {
const statusCode = error.statusCode === 500 ? 503 : error.statusCode || 503;
Copy link

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

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

return response.custom({ statusCode, body: error.message });

Check warning on line 125 in src/plugins/query_enhancements/server/routes/data_source_connection/routes.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L125 was not covered by tests
}
}
);
}
2 changes: 1 addition & 1 deletion src/plugins/query_enhancements/server/utils/plugins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export const OpenSearchEnhancements = (client: any, config: any, components: any
});

enhancements.deleteJob = createAction(client, components, {
endpoint: `${URI.ASYNC_QUERY}/<%=queryId%>`,
endpoint: `${URI.ASYNC_QUERY}`,
method: 'DELETE',
paramKey: 'queryId',
});
Expand Down
Loading