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.

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

Cancel background Glue partition futures on failure #16418

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

pettyjamesm
Copy link
Member

Description

This change ensures that GlueHiveMetastore cancels any ongoing parallel partition loading operations if a failure is encountered in one background thread or if the query is canceled. Previously, these threads would continue to run in the background unnecessarily.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Mar 7, 2023
@pettyjamesm pettyjamesm requested review from findepi and findinpath and removed request for findepi March 7, 2023 19:40
@github-actions github-actions bot added hive Hive connector tests:hive labels Mar 7, 2023
@pettyjamesm pettyjamesm requested a review from findepi March 7, 2023 22:41
@pettyjamesm pettyjamesm force-pushed the cancel-glue-futures-on-failure branch 2 times, most recently from 7247571 to aef140e Compare March 8, 2023 21:50
@pettyjamesm
Copy link
Member Author

@findepi - this change is ready for review, the test failure was caused by a race-prone assertion in TestThrottledAsyncQueue that depends on wall clock time through the RateLimiter and is unrelated to this change.

@pettyjamesm pettyjamesm force-pushed the cancel-glue-futures-on-failure branch 2 times, most recently from ec4cd36 to 98ddf48 Compare March 9, 2023 14:51
@pettyjamesm pettyjamesm force-pushed the cancel-glue-futures-on-failure branch from 98ddf48 to 9034422 Compare March 9, 2023 14:54
@findepi findepi requested review from sopel39 and arhimondr March 9, 2023 21:47
Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

The change looks good to me. However I wonder why ExecutorCompletionService is necessary? I'm thinking if it would be possible to simply use MoreFutures#allAsListWithCancellationOnFailure from Airlift?

@pettyjamesm
Copy link
Member Author

The change looks good to me. However I wonder why ExecutorCompletionService is necessary? I'm thinking if it would be possible to simply use MoreFutures#allAsListWithCancellationOnFailure from Airlift?

I think the intention is to allow processing of each future as soon as it's available without waiting for all to complete, and since the sub-futures are not pre-sorted in any meaningful way, we don't need the ordering semantics.

@arhimondr arhimondr merged commit 0675b4a into trinodb:master Mar 13, 2023
@arhimondr
Copy link
Contributor

From offline discussion:

@pettyjamesm Pointed out that futures have to be cancelled not only when one of them fails, but also in an event of a planner thread being interrupted

@pettyjamesm pettyjamesm deleted the cancel-glue-futures-on-failure branch March 13, 2023 16:40
@github-actions github-actions bot added this to the 411 milestone Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector
Development

Successfully merging this pull request may close these issues.

4 participants