moelhoussein opened a new pull request, #7685: URL: https://github.com/apache/kyuubi/pull/7685
Fixes #7684. ### Why are the changes needed? Python operations of one session all run on a single pinned Py4J thread, and Spark local properties are thread local, so the per-operation context in `ExecutePython.withLocalProperties` is not private to the operation the way it is for SQL. Two problems follow from that. The teardown clears `kyuubi.session.user` with the empty string, while `SparkOperation.withLocalProperties` clears with `null`. `AuthZUtils.getAuthzUgi` skips the property only when it is null, so an empty user reaches `UserGroupInformation.createRemoteUser` and fails with `IllegalArgumentException: Null user`. With `kyuubi.session.user.sign.enabled` it fails earlier, as `AccessControlException: Invalid user identifier []`. The empty string was already the outlier in its own method, since the same `finally` block clears the signing keys with `null` through `clearSessionUserSign`. `SessionPythonWorker.runCode` takes the worker lock per call, so the 7 to 12 internal round-trips that apply and clear the context are not atomic. Another operation of the same session can set or clear these properties in between, which loses the session user and also lets one operation's `setJobGroup` tag another operation's jobs, so its `cancelJobGroup` cancels them. This patch clears with `null` and holds the worker lock across the whole body of `withLocalProperties`, which substitutes for the isolation the SQL path gets from running each operation on its own thread. Throughput should be unaffected, because `runCode(statement)` already holds the lock for the whole execution and same-session operations are already serialized behind it. Cancellation is unaffected, because `ExecutePython.cleanup` interrupts the worker with a signal and never takes this lock. ### How was this patch tested? Two new tests in `ExecutePythonSuite`, both of which fail on master. The first runs an operation and then probes the worker thread with a bare `runCode`, which observes exactly what the teardown left behind. Master leaves `''`, so it fails with `"['']" did not equal "[None]"`. The second holds an operation's context and checks that a second thread cannot reach the worker. On master it can, so it fails with `otherOperationRanCode.get() was true`. `AuthzSessionUserSuite` covers the authz contract on its own: an empty user throws `Null user` through `getAuthzUgi`, and a null user falls back to the current user. Local results are `ExecutePythonSuite` 2 of 2, `AuthzSessionUserSuite` 1 of 1, and `PySparkTests` 7 of 8. The single failure is `executePython support timeout`, which also fails on unmodified master in the same environment (one pass in six runs on master, zero in six on this branch). It fails in the second half of that test, where `bad_code` after a timeout returns no error, which points at response handling on the worker stream after the interrupt rather than at the context. ### Was this patch assisted by generative AI tooling? Assisted-by: Claude Opus 5 -- 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]
