Skip to content

Conversation

@beliefer
Copy link
Contributor

What changes were proposed in this pull request?

This PR propose to simplify the ReloadingX509TrustManager.

Why are the changes needed?

Currently, close or destroy ReloadingX509TrustManager depend on interrupt thread and the volatile variable running.
In fact, we can change the running to a local variable on stack and let the close operation of ReloadingX509TrustManager only depend on interrupt thread.

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 14, 2024
@beliefer beliefer requested a review from JoshRosen January 14, 2024 04:47
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 good to me.
+CC @hasnain-db

@beliefer beliefer changed the title [WIP][CORE] Simplify the ReloadingX509TrustManager by the exit operation only depend on interrupt thread. [SPARK-46717][CORE] Simplify the ReloadingX509TrustManager by the exit operation only depend on interrupt thread. Jan 15, 2024
@beliefer
Copy link
Contributor Author

cc @srowen @dongjoon-hyun @LuciferYang

@LuciferYang
Copy link
Contributor

also cc @mridulm

@srowen
Copy link
Member

srowen commented Jan 15, 2024

If the interrupt happens while it's running the reload block, will the next call to sleep() yield an InterruptedException? the logic now depends on this

@beliefer
Copy link
Contributor Author

If the interrupt happens while it's running the reload block, will the next call to sleep() yield an InterruptedException? the logic now depends on this

Yes. sleep() always check the interrupt signal.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks OK pending tests

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-46717][CORE] Simplify the ReloadingX509TrustManager by the exit operation only depend on interrupt thread. [SPARK-46717][CORE] Simplify ReloadingX509TrustManager by the exit operation only depend on interrupt thread. Jan 15, 2024
@dongjoon-hyun
Copy link
Member

Merged to master for Apache Spark 4.0.0.
Thank you, @beliefer and all.

@beliefer
Copy link
Contributor Author

@dongjoon-hyun @mridulm @srowen @LuciferYang Thank you!

@hasnain-db
Copy link
Contributor

thanks for doing this @beliefer !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants