srielau commented on PR #55977: URL: https://github.com/apache/spark/pull/55977#issuecomment-4489711627
Thanks for the thorough review @cloud-fan! Pushed a followup commit (ecf913ab) that addresses every item, including the three general observations from the review summary that weren't anchored to specific lines: - **"Serial behavior is unchanged" oversimplifies.** Expanded the `setCurrentCatalog` and `setCurrentNamespace` block comments to spell out the two new concurrent torn-state windows the split-lock pattern admits (briefly invisible old namespace; drifted `isSession` snapshot) and framed them as deliberate trade-offs against the deadlock alternative. - **`CatalogManager.reset()` still nested CM->SC.** Applied the same split-lock pattern there too so the locking contract is uniform across every CM mutator that calls back into `v1SessionCatalog`. `reset` is `private[sql]` and test-only today, but the asymmetry was a footgun for future test helpers; symmetric pattern + a doc comment explains why. - **`currentPathString` is the only remaining intentional CM->SC nest.** Added a doc note on `currentPathString` explicitly labeling it as such and pointing out that any future change introducing a new SC->CM ordering must account for it so the SPARK-56939 deadlock cannot resurface via `CURRENT_PATH()` readers. - **Stale references in `SessionCatalog`.** Refreshed the comments on `lookupBuiltinOrTempTableFunction` and `lookupFunctionInfo` so they describe the post-SPARK-56939 invariant (catalog lock must never be held while reaching into `CatalogManager` from a function-resolution path) rather than citing the now-removed `setCurrentNamespace` counter-party. The regression test was extended with a third `USE`-catalog thread (see inline reply) so both arms of the split-lock pattern are exercised symmetrically. All suites green locally: `SetPathSuite` 60/60, catalyst catalog suites 119/119, `SQLFunctionSuite` + `SQLViewSuite` 70/70. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
