-
Notifications
You must be signed in to change notification settings - Fork 237
[AURON #2386] Resolve deserialized expression classes with an explicit class loader #2395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
7f0eea2
cb5244e
af806de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,23 @@ on: | |
| required: false | ||
| type: string | ||
| default: '' | ||
| assert-no-classcastexception: | ||
| description: >- | ||
| Whether to fail the job if the TPC-DS run logs any ClassCastException. Spark retries | ||
| absorb task-level deserialization failures, so queries can report PASS while throwing | ||
| hundreds of exceptions; this asserts on the log instead. | ||
| required: false | ||
| type: string | ||
| default: 'false' | ||
| jar-on-system-classpath: | ||
| description: >- | ||
| Whether to also copy the Auron jar into $SPARK_HOME/jars. When true the jar is | ||
| loaded by the application class loader; when false it reaches the JVM only through | ||
| spark-submit --jars, i.e. Spark's MutableURLClassLoader. Some class-loading defects | ||
| only reproduce in the latter configuration. | ||
| required: false | ||
| type: string | ||
| default: 'true' | ||
| queries: | ||
| description: 'Optional list of queries to run' | ||
| required: false | ||
|
|
@@ -257,12 +274,18 @@ jobs: | |
| path: dev/tpcds_1g | ||
|
|
||
| - name: Install Auron JAR | ||
| env: | ||
| JAR_ON_SYSTEM_CLASSPATH: ${{ inputs.jar-on-system-classpath }} | ||
| run: | | ||
| ls -la | ||
| jar=$(ls -1 auron-${{ inputs.sparkver }}_${{ inputs.scalaver }}*.jar | head -n1) | ||
| [ -n "$jar" ] || { echo "No jar matched: auron-${{ inputs.sparkver }}_${{ inputs.scalaver }}*.jar"; exit 1; } | ||
| echo "AURON_SPARK_JAR=$jar" >> "$GITHUB_ENV" | ||
| cp "$jar" spark-bin-${{ inputs.sparkver }}_${{ inputs.scalaver }}/jars/ | ||
| if [ "$JAR_ON_SYSTEM_CLASSPATH" = "false" ]; then | ||
| echo "Auron reaches the JVM only through spark-submit --jars (MutableURLClassLoader)" | ||
| else | ||
| cp "$jar" spark-bin-${{ inputs.sparkver }}_${{ inputs.scalaver }}/jars/ | ||
| fi | ||
|
|
||
| - name: Setup Java and Maven cache | ||
| uses: actions/setup-java@v5 | ||
|
|
@@ -380,13 +403,30 @@ jobs: | |
| SPARK_HOME: spark-bin-${{ inputs.sparkver }}_${{ inputs.scalaver }} | ||
| run: | | ||
| ls -la | ||
| set -o pipefail | ||
| dev/auron-it/run-it.sh \ | ||
| ${{ inputs.extrasparkconf }} \ | ||
| --type tpcds \ | ||
| --data-location dev/tpcds_1g \ | ||
| --query-filter ${{ matrix.query }} \ | ||
| --result-check \ | ||
| --plan-check | ||
| --plan-check 2>&1 | tee tpcds-run-${{ matrix.query }}.log | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a positive check that the runtime bloom-filter ScalarSubquery was actually injected? Right now the job only checks that no ClassCastException occurred. Also I think --plan-check is skipped for Spark 4.1, since PlanStabilityChecker currently supports only Spark 3.5. Without a positive assertion, the job could pass simply because the optimizer stopped producing the plan that triggers this path .. |
||
|
|
||
| # Task-level deserialization failures are absorbed by Spark's task retries, so the | ||
| # queries can still report PASS while throwing hundreds of exceptions. Assert on the | ||
| # log directly, otherwise a regression of AURON #2386 goes unnoticed. | ||
| - name: Assert no deserialization ClassCastException | ||
| if: ${{ inputs.assert-no-classcastexception == 'true' }} | ||
| env: | ||
| QUERY_LOG: tpcds-run-${{ matrix.query }}.log | ||
| run: | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this run only the query or small query set known to reproduce the issue and match the specific deserialization failure signature? Running all 99 TPC-DS queries adds substantial CI cost, while grepping every ClassCastException can attribute unrelated failures to expression deserialization. I think something more focused might be better here? |
||
| count=$(grep -c 'ClassCastException' "$QUERY_LOG" || true) | ||
| if [ "$count" -gt 0 ]; then | ||
| echo "::error::$count ClassCastException(s) during expression deserialization" | ||
| grep -m5 'ClassCastException' "$QUERY_LOG" || true | ||
| exit 1 | ||
| fi | ||
| echo "No ClassCastException found." | ||
|
|
||
| - name: Upload RSS log | ||
| if: ${{ failure() && (inputs.celebornver != '' || inputs.unifflever != '') }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,10 @@ package org.apache.spark.sql.auron | |
|
|
||
| import java.io.ByteArrayInputStream | ||
| import java.io.ByteArrayOutputStream | ||
| import java.io.InputStream | ||
| import java.io.ObjectInputStream | ||
| import java.io.ObjectOutputStream | ||
| import java.io.ObjectStreamClass | ||
|
|
||
| import scala.collection.mutable | ||
| import scala.jdk.CollectionConverters._ | ||
|
|
@@ -1505,24 +1507,63 @@ object NativeConverters extends Logging { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * ObjectInputStream that resolves classes against an explicit class loader. | ||
| * | ||
| * The default ObjectInputStream.resolveClass resolves each class through | ||
| * VM.latestUserDefinedLoader(), which selects a loader from the live call stack rather than the | ||
| * context class loader. During a nested read the most recent user-defined frame is often a | ||
| * Spark or Scala class, whose loader cannot see Auron classes when Auron is supplied through | ||
| * spark.jars and therefore loaded by MutableURLClassLoader. The expression graph then resolves | ||
| * only partially and an un-readResolve'd DefaultSerializationProxy is assigned into | ||
| * RDD.dependencies_, raising a ClassCastException. Pinning the loader keeps resolution | ||
| * independent of the call stack. Spark's own JavaDeserializationStream does the same. | ||
| */ | ||
| private class AuronObjectInputStream(in: InputStream, loader: ClassLoader) | ||
| extends ObjectInputStream(in) { | ||
|
|
||
| // scalastyle:off classforname | ||
| private def load(name: String, cl: ClassLoader): Class[_] = Class.forName(name, false, cl) | ||
| // scalastyle:on classforname | ||
|
|
||
| // resolveProxyClass is deliberately not overridden: the only non-deprecated way to obtain a | ||
| // proxy Class is Proxy.getProxyClass, and serialized expressions contain no dynamic proxies. | ||
| override def resolveClass(desc: ObjectStreamClass): Class[_] = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain more of your rationale here for not overriding resolveProxyClass? IIUC, the deserializeExpression is shared by UDF, UDAF, and UDTF expression graphs, which may contain serializable dynamic proxies...leaving proxy resolution on ObjectInputStream ’s default path reintroduces the stack-dependent class-loader behavior this change is intended to avoid, so proxy interfaces available only through Spark’s MutableURLClassLoader may still fail to deserialize when supplied through --jars, right? |
||
| val name = desc.getName | ||
| try { | ||
| load(name, loader) | ||
| } catch { | ||
| case _: ClassNotFoundException => | ||
| // the loader that defined Auron always resolves Auron's own classes even when the | ||
| // context loader cannot, and its parent chain still covers Spark and Scala classes | ||
| try { | ||
| load(name, getClass.getClassLoader) | ||
| } catch { | ||
| case _: ClassNotFoundException => super.resolveClass(desc) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def deserializeExpression[E <: Expression, S <: Serializable]( | ||
| serialized: Array[Byte]): (E with Serializable, S) = { | ||
| Utils.tryWithResource(new ByteArrayInputStream(serialized)) { bis => | ||
| Utils.tryWithResource(new ObjectInputStream(bis)) { ois => | ||
| def read(): (E with Serializable, S) = { | ||
| val expr = ois.readObject().asInstanceOf[E with Serializable] | ||
| val payload = ois.readObject().asInstanceOf[S with Serializable] | ||
| (expr, payload) | ||
| } | ||
| // Spark TaskMetrics#externalAccums is not thread-safe | ||
| val taskContext = TaskContext.get() | ||
| if (taskContext != null) { | ||
| taskContext.taskMetrics().synchronized { | ||
| Utils.tryWithResource(new AuronObjectInputStream(bis, Utils.getContextOrSparkClassLoader)) { | ||
| ois => | ||
| def read(): (E with Serializable, S) = { | ||
| val expr = ois.readObject().asInstanceOf[E with Serializable] | ||
| val payload = ois.readObject().asInstanceOf[S with Serializable] | ||
| (expr, payload) | ||
| } | ||
| // Spark TaskMetrics#externalAccums is not thread-safe | ||
| val taskContext = TaskContext.get() | ||
| if (taskContext != null) { | ||
| taskContext.taskMetrics().synchronized { | ||
| read() | ||
| } | ||
| } else { | ||
| read() | ||
| } | ||
| } else { | ||
| read() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure this fully guarantees that Auron is loaded only through --jars . The auron-it shaded JAR depends on the Auron uber JAR, so it looks like it may contain the same Auron classes itself. If that’s the case, removing the copy from $SPARK_HOME/jars may not reliably reproduce the original class-loader setup.
Would it make sense to exclude Auron from the integration-test fat JAR, or add a small runtime check that prints/asserts the actual class loader and code source?