You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/25 05:59:33 UTC
[GitHub] [spark] rangadi opened a new pull request, #38384: [SPARK-40657] Protobuf : Require shading for Java class jar, improve error handling
rangadi opened a new pull request, #38384:
URL: https://github.com/apache/spark/pull/38384
### What changes were proposed in this pull request?
The protobuf package shades _com.google.protobuf_ classes. That implies that the user jar providing Java classes also needs to be shaded in the same manner. Otherwise, it does not work as the `Descriptor` class used in here and the one provided by the does not match. In fact, most likely the Protobuf class can't even be instantiated.
This is an inconvenience and unintuitive for the users. But we can't make this work without shading as Spark already includes an old version of _protobuf-java_ library (2.5.0) and it will conflict with user's jar.
This improves error handling to detect various mismatches and provides more descriptive error message.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Tested manually with shaded and unshaded jars.
- How can we test with shaded classes in unit tests?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1313158714
@HeartSaVioR could you add @SandishKumarHN to the reviewers? I am not able add.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1023371786
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
+ )
+ val explanation =
+ if (unshadedMessageClass.isAssignableFrom(protobufClass)) {
+ s"$protobufClassName does not extend shaded Protobuf Message class " +
+ s"${shadedMessageClass.getName}. $missingShadingErrorMessage"
+ } else s"$protobufClassName is not a Protobuf Message type"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation)
}
// Extract the descriptor from Protobuf message.
- protobufClass
- .getDeclaredMethod("getDescriptor")
+ val getDescriptorMethod = try {
+ protobufClass
+ .getDeclaredMethod("getDescriptor")
+ } catch {
+ case _: NoSuchMethodError => // This is usually not expected.
+ throw new IllegalArgumentException(
Review Comment:
Done.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1163566789
##########
connector/protobuf/pom.xml:
##########
@@ -93,6 +93,7 @@
<artifactId>maven-shade-plugin</artifactId>
<configuration>
<shadedArtifactAttached>false</shadedArtifactAttached>
+ <shadeTestJar>true</shadeTestJar>
Review Comment:
@rangadi May I ask why `shadeTestJar` is configured to be true here? From the compilation logs, it can be seen that Maven shade does not have any tests jars being shaded, but sometimes, it will try to download non-existent jar packages, which may affect compilation speed
```
[INFO] --- shade:3.4.1:shade (default) @ spark-protobuf_2.12 ---
[INFO] Including com.google.protobuf:protobuf-java:jar:3.22.0 in the shaded jar.
Downloading from jitpack.io: https://jitpack.io/com/google/protobuf/protobuf-java/3.22.0/protobuf-java-3.22.0-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/com/google/protobuf/protobuf-java/3.22.0/protobuf-java-3.22.0-tests.jar
[WARNING] Could not get tests for com.google.protobuf:protobuf-java:jar:3.22.0:compile
[INFO] Excluding org.apache.orc:orc-core:jar:shaded-protobuf:1.8.3 from the shaded jar.
[INFO] Excluding org.apache.orc:orc-shims:jar:1.8.3 from the shaded jar.
[INFO] Excluding io.airlift:aircompressor:jar:0.21 from the shaded jar.
[INFO] Excluding org.jetbrains:annotations:jar:17.0.0 from the shaded jar.
[INFO] Excluding org.threeten:threeten-extra:jar:1.7.1 from the shaded jar.
[INFO] Excluding org.apache.orc:orc-mapreduce:jar:shaded-protobuf:1.8.3 from the shaded jar.
[INFO] Excluding org.apache.hive:hive-storage-api:jar:2.8.1 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-column:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-common:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-encoding:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.yetus:audience-annotations:jar:0.13.0 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-hadoop:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-format-structures:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-jackson:jar:1.12.3 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-databind:jar:2.14.2 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-annotations:jar:2.14.2 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-core:jar:2.14.2 from the shaded jar.
[INFO] Excluding org.apache.avro:avro:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.apache.avro:avro-mapred:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.apache.avro:avro-ipc:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.tukaani:xz:jar:1.9 from the shaded jar.
[INFO] Excluding org.apache.hadoop:hadoop-client-api:jar:3.3.5 from the shaded jar.
[INFO] Excluding org.apache.hadoop:hadoop-client-runtime:jar:3.3.5 from the shaded jar.
[INFO] Excluding commons-logging:commons-logging:jar:1.1.3 from the shaded jar.
[INFO] Excluding javax.activation:activation:jar:1.1.1 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-recipes:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-framework:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-client:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.zookeeper:zookeeper:jar:3.6.3 from the shaded jar.
[INFO] Excluding org.apache.zookeeper:zookeeper-jute:jar:3.6.3 from the shaded jar.
[INFO] Excluding org.apache.commons:commons-compress:jar:1.22 from the shaded jar.
[INFO] Excluding org.apache.commons:commons-lang3:jar:3.12.0 from the shaded jar.
[INFO] Excluding com.google.code.findbugs:jsr305:jar:3.0.0 from the shaded jar.
[INFO] Excluding org.slf4j:slf4j-api:jar:2.0.7 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-slf4j2-impl:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-api:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-core:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-1.2-api:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.xerial.snappy:snappy-java:jar:1.1.9.1 from the shaded jar.
[INFO] Excluding com.github.luben:zstd-jni:jar:1.5.4-2 from the shaded jar.
[INFO] Excluding org.scala-lang:scala-library:jar:2.12.17 from the shaded jar.
[INFO] Excluding org.apache.spark:spark-tags_2.12:jar:3.5.0-SNAPSHOT from the shaded jar.
[INFO] Excluding org.spark-project.spark:unused:jar:1.0.0 from the shaded jar.
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
[WARNING] Could not get tests for org.apache.hadoop:hadoop-client-api:jar:3.3.5:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/tukaani/xz/1.9/xz-1.9-tests.jar
[WARNING] Could not get tests for org.tukaani:xz:jar:1.9:compile
[WARNING] Could not get tests for com.google.protobuf:protobuf-java:jar:3.22.0:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
[WARNING] Could not get tests for org.apache.curator:curator-framework:jar:2.13.0:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/logging/log4j/log4j-slf4j2-impl/2.20.0/log4j-slf4j2-impl-2.20.0-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/logging/log4j/log4j-slf4j2-impl/2.20.0/log4j-slf4j2-impl-2.20.0-tests.jar
...
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by "rangadi (via GitHub)" <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1163659080
##########
connector/protobuf/pom.xml:
##########
@@ -93,6 +93,7 @@
<artifactId>maven-shade-plugin</artifactId>
<configuration>
<shadedArtifactAttached>false</shadedArtifactAttached>
+ <shadeTestJar>true</shadeTestJar>
Review Comment:
Oh, it is trying to download many other test jars?
We can remove this config. Originally I enabled it so that we have some shaded protobuf classes to experiment with.
We don't need it any more. We can use https://github.com/rangadi/shaded-protobuf-classes to create custom shaded jars.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1313157909
@SandishKumarHN could you take a look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024522973
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Ah OK, this is to help jar shader to not shade it... Good to know. Probably much helpful to have a code comment explaining this for future reader.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024522973
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Ah OK, this is to avoid jar shader to shade it... Good to know. Probably much helpful to have a code comment explaining this for future reader.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024513027
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
Review Comment:
Oops! Will fix in a follow up.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1290299192
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1317660304
Thanks! Merging to master.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024518159
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
This is is necessary. Net effect we want "com.google.protobuf.Message", but if we have the string directly, jar shader replaces that also with "org.sparkproject.spark-protobuf.protobuf.Message"!
That's why we need to trick it with `replace()`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1317663388
(I've just realized that this PR is a follow-up with already resolved JIRA ticket. Please add the prefix `[FOLLOWUP]` for such case. If the change is non-trivial, we advise to create a new JIRA ticket.)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1163566789
##########
connector/protobuf/pom.xml:
##########
@@ -93,6 +93,7 @@
<artifactId>maven-shade-plugin</artifactId>
<configuration>
<shadedArtifactAttached>false</shadedArtifactAttached>
+ <shadeTestJar>true</shadeTestJar>
Review Comment:
@rangadi May I ask why `shadeTestJar` is configured to be true here? From the compilation logs, it can be seen that Maven shade does not have any tests jars being shaded, but they always try to download non-existent jar packages, which may affect compilation speed
```
[INFO] --- shade:3.4.1:shade (default) @ spark-protobuf_2.12 ---
[INFO] Including com.google.protobuf:protobuf-java:jar:3.22.0 in the shaded jar.
Downloading from jitpack.io: https://jitpack.io/com/google/protobuf/protobuf-java/3.22.0/protobuf-java-3.22.0-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/com/google/protobuf/protobuf-java/3.22.0/protobuf-java-3.22.0-tests.jar
[WARNING] Could not get tests for com.google.protobuf:protobuf-java:jar:3.22.0:compile
[INFO] Excluding org.apache.orc:orc-core:jar:shaded-protobuf:1.8.3 from the shaded jar.
[INFO] Excluding org.apache.orc:orc-shims:jar:1.8.3 from the shaded jar.
[INFO] Excluding io.airlift:aircompressor:jar:0.21 from the shaded jar.
[INFO] Excluding org.jetbrains:annotations:jar:17.0.0 from the shaded jar.
[INFO] Excluding org.threeten:threeten-extra:jar:1.7.1 from the shaded jar.
[INFO] Excluding org.apache.orc:orc-mapreduce:jar:shaded-protobuf:1.8.3 from the shaded jar.
[INFO] Excluding org.apache.hive:hive-storage-api:jar:2.8.1 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-column:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-common:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-encoding:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.yetus:audience-annotations:jar:0.13.0 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-hadoop:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-format-structures:jar:1.12.3 from the shaded jar.
[INFO] Excluding org.apache.parquet:parquet-jackson:jar:1.12.3 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-databind:jar:2.14.2 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-annotations:jar:2.14.2 from the shaded jar.
[INFO] Excluding com.fasterxml.jackson.core:jackson-core:jar:2.14.2 from the shaded jar.
[INFO] Excluding org.apache.avro:avro:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.apache.avro:avro-mapred:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.apache.avro:avro-ipc:jar:1.11.1 from the shaded jar.
[INFO] Excluding org.tukaani:xz:jar:1.9 from the shaded jar.
[INFO] Excluding org.apache.hadoop:hadoop-client-api:jar:3.3.5 from the shaded jar.
[INFO] Excluding org.apache.hadoop:hadoop-client-runtime:jar:3.3.5 from the shaded jar.
[INFO] Excluding commons-logging:commons-logging:jar:1.1.3 from the shaded jar.
[INFO] Excluding javax.activation:activation:jar:1.1.1 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-recipes:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-framework:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.curator:curator-client:jar:2.13.0 from the shaded jar.
[INFO] Excluding org.apache.zookeeper:zookeeper:jar:3.6.3 from the shaded jar.
[INFO] Excluding org.apache.zookeeper:zookeeper-jute:jar:3.6.3 from the shaded jar.
[INFO] Excluding org.apache.commons:commons-compress:jar:1.22 from the shaded jar.
[INFO] Excluding org.apache.commons:commons-lang3:jar:3.12.0 from the shaded jar.
[INFO] Excluding com.google.code.findbugs:jsr305:jar:3.0.0 from the shaded jar.
[INFO] Excluding org.slf4j:slf4j-api:jar:2.0.7 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-slf4j2-impl:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-api:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-core:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.apache.logging.log4j:log4j-1.2-api:jar:2.20.0 from the shaded jar.
[INFO] Excluding org.xerial.snappy:snappy-java:jar:1.1.9.1 from the shaded jar.
[INFO] Excluding com.github.luben:zstd-jni:jar:1.5.4-2 from the shaded jar.
[INFO] Excluding org.scala-lang:scala-library:jar:2.12.17 from the shaded jar.
[INFO] Excluding org.apache.spark:spark-tags_2.12:jar:3.5.0-SNAPSHOT from the shaded jar.
[INFO] Excluding org.spark-project.spark:unused:jar:1.0.0 from the shaded jar.
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/apache/hadoop/hadoop-client-api/3.3.5/hadoop-client-api-3.3.5-tests.jar
[WARNING] Could not get tests for org.apache.hadoop:hadoop-client-api:jar:3.3.5:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/tukaani/xz/1.9/xz-1.9-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/tukaani/xz/1.9/xz-1.9-tests.jar
[WARNING] Could not get tests for org.tukaani:xz:jar:1.9:compile
[WARNING] Could not get tests for com.google.protobuf:protobuf-java:jar:3.22.0:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from gcs-maven-central-mirror: https://maven-central.storage-download.googleapis.com/maven2/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from apache.snapshots.https: https://repository.apache.org/content/repositories/snapshots/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from repository.jboss.org: https://repository.jboss.org/nexus/content/groups/public/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from sonatype-releases: https://oss.sonatype.org/content/repositories/releases/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from jitpack.io: https://jitpack.io/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
Downloading from ossrh: https://oss.sonatype.org/content/repositories/snapshots/org/apache/curator/curator-framework/2.13.0/curator-framework-2.13.0-tests.jar
[WARNING] Could not get tests for org.apache.curator:curator-framework:jar:2.13.0:compile
Downloading from aliyun-nexus: https://maven.aliyun.com/nexus/content/groups/public/org/apache/logging/log4j/log4j-slf4j2-impl/2.20.0/log4j-slf4j2-impl-2.20.0-tests.jar
Downloading from aliyun-central: https://maven.aliyun.com/repository/central/org/apache/logging/log4j/log4j-slf4j2-impl/2.20.0/log4j-slf4j2-impl-2.20.0-tests.jar
...
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024692244
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Updated this in #38680. It adds an explanation and uses join rather than replace().
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR closed pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR closed pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
URL: https://github.com/apache/spark/pull/38384
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024522973
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Ah OK, this is to help jar shader to shade it... Good to know. Probably much helpful to have a code comment explaining this for future reader.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SandishKumarHN commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
SandishKumarHN commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1021864869
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
+ )
+ val explanation =
+ if (unshadedMessageClass.isAssignableFrom(protobufClass)) {
+ s"$protobufClassName does not extend shaded Protobuf Message class " +
+ s"${shadedMessageClass.getName}. $missingShadingErrorMessage"
+ } else s"$protobufClassName is not a Protobuf Message type"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation)
}
// Extract the descriptor from Protobuf message.
- protobufClass
- .getDeclaredMethod("getDescriptor")
+ val getDescriptorMethod = try {
+ protobufClass
+ .getDeclaredMethod("getDescriptor")
+ } catch {
+ case _: NoSuchMethodError => // This is usually not expected.
+ throw new IllegalArgumentException(
Review Comment:
can we move this to a new error class framework?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on pull request #38384: [SPARK-40657] Protobuf : Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1290028753
cc: @SandishKumarHN @mposdev21
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1313207941
We can't add reviewers who aren't a part of committers of the project. The best bet is to let them know via cc. and quickly leave a review comment which will register them to the reviewer.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024522973
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Ah OK, this is to avoid jar shader to shade it... Good to know. Probably much helpful to have a code comment explaining this for future reader. Shall we add the code comment in the followup PR as well? Thanks in advance!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024506647
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
Do we have specific reason to not just say "com.google.protobuf.Message"? It seems to me technically the same. If it is just an informal one, shall we consider code comment instead?
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
Review Comment:
nit: `the the the` doesn't seem to be what we want to say. shall we correct this via follow-up PR?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on a diff in pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on code in PR #38384:
URL: https://github.com/apache/spark/pull/38384#discussion_r1024614312
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/utils/ProtobufUtils.scala:
##########
@@ -155,21 +155,52 @@ private[sql] object ProtobufUtils extends Logging {
* Loads the given protobuf class and returns Protobuf descriptor for it.
*/
def buildDescriptorFromJavaClass(protobufClassName: String): Descriptor = {
+
+ // Default 'Message' class here is shaded while using the package (as in production).
+ // The incoming classes might not be shaded. Check both.
+ val shadedMessageClass = classOf[Message] // Shaded in prod, not in unit tests.
+ val missingShadingErrorMessage = "The jar with Protobuf classes needs to be shaded " +
+ s"(com.google.protobuf.* --> ${shadedMessageClass.getPackage.getName}.*)"
+
val protobufClass = try {
Utils.classForName(protobufClassName)
} catch {
case e: ClassNotFoundException =>
- throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, e)
+ val explanation =
+ if (protobufClassName.contains(".")) "Ensure the class include in the jar"
+ else "Ensure the class name includes package prefix"
+ throw QueryCompilationErrors.protobufClassLoadError(protobufClassName, explanation, e)
+
+ case e: NoClassDefFoundError if e.getMessage.matches("com/google/proto.*Generated.*") =>
+ // This indicates the the the Java classes are not shaded.
+ throw QueryCompilationErrors.protobufClassLoadError(
+ protobufClassName, missingShadingErrorMessage, e)
}
- if (!classOf[Message].isAssignableFrom(protobufClass)) {
- throw QueryCompilationErrors.protobufMessageTypeError(protobufClassName)
- // TODO: Need to support V2. This might work with V2 classes too.
+ if (!shadedMessageClass.isAssignableFrom(protobufClass)) {
+ // Check if this extends 2.x Message class included in spark, that does not work.
+ val unshadedMessageClass = Utils.classForName(
+ "com.escape-shading.google.protobuf.Message".replace("escape-shading.", "")
Review Comment:
will do that.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] rangadi commented on pull request #38384: [SPARK-40657][PROTOBUF] Require shading for Java class jar, improve error handling
Posted by GitBox <gi...@apache.org>.
rangadi commented on PR #38384:
URL: https://github.com/apache/spark/pull/38384#issuecomment-1317351559
@HeartSaVioR or @HyukjinKwon please merge this.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org