use jackson for @JsonSerialize annotated types #174
Conversation
Some types (e.g. beam PipelineOptions) are not kryo nor java serializable, but they are jackson serializable.
|
Addresses #170 |
|
Not sure why tests fail, works on my laptop! 😄 |
|
For some reason |
|
Looks cool to me (pun intended... I don't know much about Kryo and what effects |
|
@ulzha kryo registration required already defaults to false. I just kept the explicit call because the chill-scala kryo setup was explicitly setting it. |
Codecov Report
@@ Coverage Diff @@
## master #174 +/- ##
===========================================
+ Coverage 69.59% 69.7% +0.11%
- Complexity 363 376 +13
===========================================
Files 67 68 +1
Lines 1618 1654 +36
Branches 110 112 +2
===========================================
+ Hits 1126 1153 +27
- Misses 425 433 +8
- Partials 67 68 +1 |
| kryo = new Kryo(); | ||
| kryo = new Kryo() { | ||
| @Override | ||
| public Registration getRegistration(Class type) { |
There was a problem hiding this comment.
It isn't completely valid to override getRegistration, because it looks more like default serializer. There is getDefaultSerializerForAnnotatedType to override.
There was a problem hiding this comment.
For instance, this will make not possible to register custom serializers for anything that JacksonJsonSerializer can comprehend.
There was a problem hiding this comment.
Unfortunately Kryo special handles Proxy instances in a way which means that getDefaultSerializerForAnnotatedType can't be used to handle PipelineOptions.
| <dependency> | ||
| <groupId>com.twitter</groupId> | ||
| <artifactId>chill_2.11</artifactId> | ||
| <version>0.9.2</version> |
There was a problem hiding this comment.
It's an awkward dependency to have in the parent pom because then we should move the chill dependencies from the scala submodules as well into the parent pom and then we end up enumerating the scala versions in the parent pom... do we want that?
| : Optional.empty(); | ||
| } | ||
|
|
||
| static void register(Kryo kryo) { |
There was a problem hiding this comment.
Nitpicking, this method reads a bit strange. Will it be better called registerTo?
There was a problem hiding this comment.
You have nominated yourself for a nitpick award ;P
| } | ||
| } | ||
|
|
||
| private static class ScalaKryoSetup { |
There was a problem hiding this comment.
What's the benefit having a static class here instead of just a static method? And why is the branch using chill-java inlined instead of being factored out?
There was a problem hiding this comment.
It needs to be a static class because we don't want the method to be class-loaded before we know that we have chill-scala available.
There was a problem hiding this comment.
|
👍 |
|
Oh, wait, please fix coverage. |
|
Let's close this in favour of #178 ? |
Hey, I just made a Pull Request!
Description
use jackson serializaion for @JsonSerialize annotated types
Motivation and Context
Some types (e.g. beam
PipelineOptions) are not kryo nor javaserializable, but they are jackson serializable.
Have you tested this? If so, how?
Added a test for
PipelineOptions.Checklist for PR author(s)
Checklist for PR reviewer(s)