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

[SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. #44732

Closed
wants to merge 2 commits into from

Conversation

beliefer
Copy link
Contributor

@beliefer beliefer commented Jan 15, 2024

What changes were proposed in this pull request?

This PR propose to simplify the BlockManager.

Why are the changes needed?

Currently, close or destroy BlockManager depend on interrupt thread and the volatile variable stopped.
In fact, we can change the stopped to a local variable on stack and let the close operation of BlockManager only depend on interrupt thread.
For further optimization, this PR using running instead of stopped.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

GA tests.

Was this patch authored or co-authored using generative AI tooling?

'No'.

@github-actions github-actions bot added the CORE label Jan 15, 2024
@beliefer beliefer changed the title [WIP][CORE] Simplify the ContextCleaner by the exit operation only depend on interrupt thread. [WIP][CORE] Simplify the ContextCleaner|BlockManager by the exit operation only depend on interrupt thread. Jan 15, 2024
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Unlike ReloadingX509TrustManager , here the InterruptedException is caught and ignored in some of the downstream methods: so we cant rely on that pattern here.

@beliefer beliefer changed the title [WIP][CORE] Simplify the ContextCleaner|BlockManager by the exit operation only depend on interrupt thread. [SPARK-46733][CORE] Simplify the ContextCleaner|BlockManager by the exit operation only depend on interrupt thread. Jan 16, 2024
@beliefer beliefer changed the title [SPARK-46733][CORE] Simplify the ContextCleaner|BlockManager by the exit operation only depend on interrupt thread. [SPARK-46733][CORE] Simplify the BlockManager by the exit operation only depend on interrupt thread. Jan 16, 2024
@beliefer
Copy link
Contributor Author

Unlike ReloadingX509TrustManager , here the InterruptedException is caught and ignored in some of the downstream methods: so we cant rely on that pattern here.

I reverted the ContextCleaner.

Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.
+CC @dongjoon-hyun as well who reviewed the prev pr.

@beliefer
Copy link
Contributor Author

@LuciferYang
Copy link
Contributor

This change of this pr is fine to me.

However, I would like to raise a question: Would it be simpler and clearer to implement the logic for cleaning up the DownloadFile resource using the java.lang.ref.Cleaner mechanism instead of the current approach?

@beliefer
Copy link
Contributor Author

Java.lang.ref.Cleaner cannot respond to interrupts.

@beliefer
Copy link
Contributor Author

cc @mridulm
Could you take a review again?

@mridulm
Copy link
Contributor

mridulm commented Jan 23, 2024

Since you looked into this - do you have any other thoughts on this @LuciferYang ? Thanks

@LuciferYang
Copy link
Contributor

no more, the change is fine to me

Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

LGTM

@mridulm mridulm closed this in ee6ed43 Jan 24, 2024
@mridulm
Copy link
Contributor

mridulm commented Jan 24, 2024

Merged to master.
Thanks for working on this @beliefer !
Thanks for the review @LuciferYang :-)

@beliefer
Copy link
Contributor Author

@mridulm @LuciferYang Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants