[BEAM-1144] Spark runner fails to deserialize MicrobatchSource in cluster mode#1613
[BEAM-1144] Spark runner fails to deserialize MicrobatchSource in cluster mode#1613aviemzur wants to merge 2 commits intoapache:masterfrom
Conversation
| <groupId>com.esotericsoftware.kryo</groupId> | ||
| <artifactId>kryo</artifactId> | ||
| <version>2.21.1</version> | ||
| <version>2.21</version> |
| public void registerClasses(Kryo kryo) { | ||
| for (Class<?> clazz : ClassesForJavaSerialization.getClasses()) { | ||
| kryo.register(clazz, new JavaSerializer()); | ||
| kryo.register(clazz, new StatelessJavaSerializer()); |
| try { | ||
| return Class.forName(desc.getName(), false, classLoader); | ||
| } catch (ClassNotFoundException e) { | ||
| throw new RuntimeException("Could not find class: " + desc.getName(), e); |
There was a problem hiding this comment.
primClasses is a private member of ObjectInputStream. In any case, primitives aren't serialized with StatelessJavaSerializer only the classes that were registered to use it (Source and Coder implementations)
| /** | ||
| * ObjectInputStream with specific ClassLoader. | ||
| */ | ||
| class ObjectInputStreamWithClassLoader extends ObjectInputStream { |
There was a problem hiding this comment.
This is only used by StatelessJavaSerializer, why not make it an inner class ?
StatelessJavaSerializer also provides explanations as to way this is needed at all.
| * Stateless Java Serializer. | ||
| * | ||
| * <p> | ||
| * Solves state re-use issue in Kryo version 2.21 used in Spark 1.x |
| * </p> | ||
| * | ||
| * <p> | ||
| * Also, solves class loading issue in cluster caused by ${@link ObjectInputStream} |
| for (Class<?> clazz : classesForJavaSerialization) { | ||
| Assert.assertThat("Registered serializer for class " + clazz.getName() | ||
| + " was not an instance of " + JavaSerializer.class.getName(), | ||
| + " was not an instance of " + StatelessJavaSerializer.class.getName(), |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@amitsela made the changes discussed in comments. |
|
LGTM from me, @jbonofre PTAL - specifically on |
|
CC: @jbonofre Thanks guys ! Let me take a look on |
|
This change exists also on |
|
Thanks @aviemzur this is reassuring. |
|
It sounds good to me, but I'm checking one more thing. |
|
Ping @jbonofre can we merge this ? |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks! |
4 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.