Skip to content

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR is a kind of a followup of #42120. This addresses my own comment at #42120 (comment) by adding the support of InheritableThread with Spark Connect.

Why are the changes needed?

In order to allow the thread local hierarchy supported in Scala side.

Does this PR introduce any user-facing change?

The main PR is not released out yet so it doesn't have user-facing change.

How was this patch tested?

Unittests were added.

:class:`threading.Thread` but correctly inherits the inheritable properties specific
to JVM thread such as ``InheritableThreadLocal``.
Also, note that pinned thread mode does not close the connection from Python
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 is already fixed in Py4J, py4j/py4j#471

@HyukjinKwon
Copy link
Member Author

cc @zhengruifeng and @WeichenXu123 I need your look here.

@zhengruifeng
Copy link
Contributor

LGTM, but I think we need @WeichenXu123 's approval for this PR

Copy link
Contributor

@WeichenXu123 WeichenXu123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@HyukjinKwon HyukjinKwon force-pushed the SPARK-44509-followup branch from b03109f to 391ce44 Compare July 25, 2023 10:37
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 (Pending CIs)

@HyukjinKwon
Copy link
Member Author

All tests passed. The failed test is pip packaging test that happens after running all unittests. I will fix them separately at #42159.

Merged to master and branch-3.5.

@HyukjinKwon HyukjinKwon deleted the SPARK-44509-followup branch January 15, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants