wenjin272 commented on code in PR #1110:
URL: https://github.com/apache/flink-agents/pull/1110#discussion_r4003178842
##########
dist/pom.xml:
##########
@@ -160,9 +160,45 @@ under the License.
<exclude>META-INF/org/apache/logging/log4j/**</exclude>
<exclude>META-INF/services/org.apache.logging.log4j.*</exclude>
<exclude>META-INF/licenses/LICENSE.slf4j*</exclude>
+ <!-- The shade plugin relocates bytecode but
not file
+ paths under META-INF/versions/**:
relocated MR entries
+ keep the original package path while
their bytecode
+ declares the shaded name, which breaks
the JDK 9+ MR
+ lookup of the original class with
NoClassDefFoundError
+ (wrong name). The base classes are kept,
so dropping the
+ MR copies is lossless. -->
+
<exclude>META-INF/versions/*/com/fasterxml/**</exclude>
</excludes>
</filter>
</filters>
+ <!--
+ Relocate third-party libraries that are commonly
present in user job JARs
+ or other JARs on the Flink classpath. When
flink-agents-dist is submitted
+ alongside user code (e.g. via the "jars" option or
placed in lib/) it lands
+ in the same ClassLoader as user code, so a version
mismatch on these
+ packages causes NoSuchMethodError or
ClassCastException.
+
+ Packages that Flink already relocates in
flink-shaded-* (jackson, guava,
+ netty, asm) are NOT repeated here: Flink's copies live
under
+ org.apache.flink.shaded.* and therefore never conflict
with ours.
+
+ kotlin / okhttp3 / okio are intentionally NOT
relocated: the shade plugin
+ applies string replacement to all resource file names,
corrupting
+ Kotlin's .kotlin_builtins metadata files. okhttp3/okio
are Kotlin-based
+ and must move together with kotlin stdlib, so all
three are left unshaded.
+ -->
+ <relocations>
+ <!-- kafka-clients is ubiquitous in Flink streaming
jobs -->
+ <relocation>
+ <pattern>org.apache.kafka</pattern>
+
<shadedPattern>org.apache.flink.agents.shaded.org.apache.kafka</shadedPattern>
+ </relocation>
+ <!-- jackson is widely used for JSON serialisation in
user code -->
+ <relocation>
+ <pattern>com.fasterxml</pattern>
+
<shadedPattern>org.apache.flink.agents.shaded.com.fasterxml</shadedPattern>
Review Comment:
Could we avoid relocating Jackson until the serialization boundary is
designed end to end? Flink can use `flink-shaded-jackson` because its Jackson
DTOs and mapper are framework-owned and compiled against the same shaded
namespace; its shaded mapper does not need to interpret
`com.fasterxml.jackson.*` annotations on user-defined classes.
Flink Agents is different: custom `Event` subclasses are a public extension
point, the documentation requires users to annotate their constructors with the
standard `@JsonCreator`/`@JsonProperty`, and `ActionStateSerde` uses Jackson
polymorphic deserialization to restore those user classes. The user job JAR is
not processed by this shade execution, so the relocated mapper will not
recognize those annotations.
This is therefore more than a shade configuration change. We first need a
complete serialization design—for example, persisting a framework-owned Event
envelope and reconstructing user types through an explicit codec/registration
contract—and packaged tests covering custom-event recovery. As implemented,
relocating all of `com.fasterxml` changes the documented serialization contract
despite the PR declaring no public API change.
##########
dist/pom.xml:
##########
@@ -160,9 +160,45 @@ under the License.
<exclude>META-INF/org/apache/logging/log4j/**</exclude>
<exclude>META-INF/services/org.apache.logging.log4j.*</exclude>
<exclude>META-INF/licenses/LICENSE.slf4j*</exclude>
+ <!-- The shade plugin relocates bytecode but
not file
+ paths under META-INF/versions/**:
relocated MR entries
+ keep the original package path while
their bytecode
+ declares the shaded name, which breaks
the JDK 9+ MR
+ lookup of the original class with
NoClassDefFoundError
+ (wrong name). The base classes are kept,
so dropping the
+ MR copies is lossless. -->
+
<exclude>META-INF/versions/*/com/fasterxml/**</exclude>
</excludes>
</filter>
</filters>
+ <!--
+ Relocate third-party libraries that are commonly
present in user job JARs
+ or other JARs on the Flink classpath. When
flink-agents-dist is submitted
+ alongside user code (e.g. via the "jars" option or
placed in lib/) it lands
+ in the same ClassLoader as user code, so a version
mismatch on these
+ packages causes NoSuchMethodError or
ClassCastException.
+
+ Packages that Flink already relocates in
flink-shaded-* (jackson, guava,
+ netty, asm) are NOT repeated here: Flink's copies live
under
+ org.apache.flink.shaded.* and therefore never conflict
with ours.
+
+ kotlin / okhttp3 / okio are intentionally NOT
relocated: the shade plugin
+ applies string replacement to all resource file names,
corrupting
+ Kotlin's .kotlin_builtins metadata files. okhttp3/okio
are Kotlin-based
+ and must move together with kotlin stdlib, so all
three are left unshaded.
+ -->
+ <relocations>
+ <!-- kafka-clients is ubiquitous in Flink streaming
jobs -->
+ <relocation>
+ <pattern>org.apache.kafka</pattern>
+
<shadedPattern>org.apache.flink.agents.shaded.org.apache.kafka</shadedPattern>
Review Comment:
Please also adjust the thin-JAR filter. Relocated classes are placed under
`org/apache/flink/agents/shaded/**`, which matches the existing
`org/apache/flink/agents/**` include and causes Kafka/Jackson classes to be
copied into every `-thin.jar`. Excluding the shaded subtree from that execution
should keep the thin JARs thin; please verify that they contain no relocated
dependency classes.
--
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]
