-
Notifications
You must be signed in to change notification settings - Fork 953
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
[Discover] Cancel S3 Queries Using SQL Plugin #9355
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sean Li <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9355 +/- ##
=======================================
Coverage 61.71% 61.71%
=======================================
Files 3816 3816
Lines 91829 91841 +12
Branches 14543 14546 +3
=======================================
+ Hits 56668 56680 +12
Misses 31506 31506
Partials 3655 3655
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Sean Li <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javascript file :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a lot of our Cypress tests are JavaScript files, is that something we want to change?
@@ -21,6 +21,8 @@ export const DS_API = { | |||
}; | |||
export const DSM_API = '/internal/data-source-management/fetchDataSourceMetaData'; | |||
|
|||
export const DELETE_API = '/api/enhancements/jobs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated to this PR but not a big fan of the multiple constants defined instead of using nested constants for similar things like APIs, when issue is the number of modifications increasing as I will have to import each constant individually.
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) |
So far, it's wired up to the point where the delete request will be sent when the query enhancement's
This is the main thing that this PR acheives.
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.
Think this will probably need a fast followup or some improvements on how I'm doing this. |
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
Check List
yarn test:jest
yarn test:jest_integration