wangzhigang1999 commented on code in PR #7685:
URL: https://github.com/apache/kyuubi/pull/7685#discussion_r3889038236
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/AuthzSessionUserSuite.scala:
##########
Review Comment:
This test verifies existing `AuthZUtils` behavior, but it does not exercise
the `ExecutePython` change and still passes when the patch is reverted. Since
`ExecutePythonSuite` already verifies that the property is cleared with null,
could we remove this suite or fold the relevant assertion into that regression
test?
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecutePython.scala:
##########
@@ -143,35 +143,44 @@ class ExecutePython(
}
override protected def withLocalProperties[T](f: => T): T = {
- try {
- // to prevent the transferred set job group python code broken
- val jobDesc = s"Python statement: $statementId"
- // for python, the boolean value is capitalized
- val pythonForceCancel = if (forceCancel) "True" else "False"
- worker.runCode(
- "spark.sparkContext.setJobGroup" +
- s"('$statementId', '$jobDesc', $pythonForceCancel)",
- internal = true)
- setSparkLocalProperty(KYUUBI_SESSION_USER_KEY, session.user)
- setSparkLocalProperty(KYUUBI_STATEMENT_ID_KEY, statementId)
- schedulerPool match {
- case Some(pool) =>
- setSparkLocalProperty(SPARK_SCHEDULER_POOL_KEY, pool)
- case None =>
- }
- if (isSessionUserSignEnabled) {
- setSessionUserSign()
- }
+ // Spark local properties are thread local, and every python operation of
a session shares
+ // one worker thread, so the context below is not private to this
operation the way it is for
+ // SQL. Holding the worker lock across apply, run and clear keeps a
concurrent operation of
+ // the same session from setting or clearing these properties in between
our round-trips.
+ worker.withLockRequired {
+ try {
+ // to prevent the transferred set job group python code broken
+ val jobDesc = s"Python statement: $statementId"
+ // for python, the boolean value is capitalized
+ val pythonForceCancel = if (forceCancel) "True" else "False"
+ worker.runCode(
+ "spark.sparkContext.setJobGroup" +
+ s"('$statementId', '$jobDesc', $pythonForceCancel)",
+ internal = true)
+ setSparkLocalProperty(KYUUBI_SESSION_USER_KEY, session.user)
+ setSparkLocalProperty(KYUUBI_STATEMENT_ID_KEY, statementId)
+ schedulerPool match {
+ case Some(pool) =>
+ setSparkLocalProperty(SPARK_SCHEDULER_POOL_KEY, pool)
+ case None =>
+ }
+ if (isSessionUserSignEnabled) {
+ setSessionUserSign()
+ }
- f
- } finally {
- setSparkLocalProperty(KYUUBI_SESSION_USER_KEY, "")
- setSparkLocalProperty(KYUUBI_STATEMENT_ID_KEY, "")
- setSparkLocalProperty(SPARK_SCHEDULER_POOL_KEY, "")
- // using cancelJobGroup for pyspark, see details in pyspark/context.py
- worker.runCode(s"spark.sparkContext.cancelJobGroup('$statementId')",
internal = true)
- if (isSessionUserSignEnabled) {
- clearSessionUserSign()
+ f
+ } finally {
+ // Clear with null, not the empty string. AuthZUtils.getAuthzUgi only
skips the property
+ // when it is null, so an empty user reaches
UserGroupInformation.createRemoteUser and
+ // fails with `Null user`, and fails session user signing as a blank
identifier.
Review Comment:
This is only true when session-user signing is disabled. When signing is
enabled, verifyKyuubiSessionUser runs first and rejects both null and empty
values. Could you clarify that the outer lock, rather than the null value,
protects the signing-enabled path?
--
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]
