Skip to content

[SPARK-49876][CONNECT] Get rid of global locks from Spark Connect Service#48350

Closed
ghost wants to merge 1 commit intoapache:masterfrom
wvwwvwwv:SPARK-49876-REMOVE-LOCKS
Closed

[SPARK-49876][CONNECT] Get rid of global locks from Spark Connect Service#48350
ghost wants to merge 1 commit intoapache:masterfrom
wvwwvwwv:SPARK-49876-REMOVE-LOCKS

Conversation

@ghost
Copy link

@ghost ghost commented Oct 4, 2024

What changes were proposed in this pull request?

Get rid of global locks from Spark Connect Service.

  • ServerSideListenerHolder: AtomicReference replaces the global lock.
  • SparkConnectStreamingQueryCache: two global locks are replaced with ConcurrentHashMap and a mutex-protected per-tag data structure, i.e., global locks -> a per-tag lock.

Why are the changes needed?

Spark Connect Service doesn't limit the number of threads, susceptible to priority inversion because of heavy use of global locks.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests + modified an existing test.

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

No.

@ghost ghost changed the title [WIP][SPARK-49876] Get rid of global locks from Spark Connect Service [SPARK-49876] Get rid of global locks from Spark Connect Service Oct 5, 2024
Copy link
Author

Choose a reason for hiding this comment

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

To whom it may concern - I'm pretty sure that this condition was wrong, so fixed it.

Copy link
Author

Choose a reason for hiding this comment

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

@HyukjinKwon can you please check if the condition was right? as far as I understood, the key was supposed to be removed if no associated entry was found in queryCache. thanks!

@ghost ghost changed the title [SPARK-49876] Get rid of global locks from Spark Connect Service [SPARK-49876][CONNECT] Get rid of global locks from Spark Connect Service Oct 5, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it has to remove the element within value instead of the whole entry IIRC.

Copy link

Choose a reason for hiding this comment

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

You're right, and the code actually does that, though it's not quite obvious; value.filter will return true only when value is empty, so semantically the piece of code means,

  1. Remove elements that do not satisfy queryCache.containsKey(k) from value.
  2. If value is empty after filtering those elements, value will be removed from taggedQueries.

Copy link
Member

Choose a reason for hiding this comment

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

ah okie, then lgtm

@ghost ghost requested a review from HyukjinKwon October 14, 2024 07:19
@ghost
Copy link
Author

ghost commented Oct 14, 2024

@rangadi Hello, nice to meet you here! This is a minor code optimization as part of an attempt to get rid of the use of global locks in the spark connect service code. Could you review the code please? The code semantics should be the same as before. Thanks.

@HyukjinKwon
Copy link
Member

Merged to master.

@ghost ghost deleted the SPARK-49876-REMOVE-LOCKS branch October 23, 2024 11:54
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.

2 participants