Repository navigation
Preserve broadcast resource IDs during concurrent registration - #13320
Conversation
a72ed0f to
351a9af
Compare
|
@JkSelf @zhztheplayer could you please take a look, thanks. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open finding
What changed in this PR
Fixes a concurrency bug in broadcast resource tracking by using a thread-safe Set implementation for per-execution resource IDs, preventing lost IDs during parallel registration.
Changes:
- Replace
java.util.HashSetwithConcurrentHashMap.newKeySet()forexecutionId -> resourceIdscache values to support concurrentadd()operations.
| File | Description |
|---|---|
| backends-velox/src/main/scala/org/apache/spark/rpc/GlutenDriverEndpoint.scala | Switch cached resource-id set to a concurrent set to avoid lost registrations under multi-threaded access |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
0 open findings
1 resolved since last review
🧠 Review effort: Lite
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open finding
🧠 Review effort: Lite
zhztheplayer
left a comment
zhztheplayer
left a comment
There was a problem hiding this comment.
Does backends-clickhouse have the same bug given the same code structure?
c25e022 to
94771f8
Compare
yikf
changed the title
Thank you, updated. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
0 open findings
1 resolved since last review
🧠 Review effort: Lite
|
Run Gluten Clickhouse CI on x86 |
94771f8 to
1242d99
Compare
|
Run Gluten Clickhouse CI on x86 |
What changes are proposed in this pull request?
Parallel broadcast plans can register resource IDs for the same SQL execution from different driver threads. The Caffeine cache is thread-safe, but its HashSet values are not, so concurrent additions can lose IDs and leave broadcast hash tables out of execution-end cleanup messages.
Use ConcurrentHashMap.newKeySet() for each execution's resource IDs. Preserve the existing expiration and cleanup flow.
How was this patch tested?
Exists UTs.
Was this patch authored or co-authored using generative AI tooling?
No.