Skip to content

More Joda serializers - #1632

Closed
fosskers wants to merge 2 commits into
locationtech:masterfrom
fosskers:fix/cgw/joda-serializers
Closed

fosskers wants to merge 2 commits into
locationtech:masterfrom
fosskers:fix/cgw/joda-serializers

Conversation

@fosskers

Copy link
Copy Markdown
Contributor

Motivation

Kryo and JodaTime do not get along. We use the kryo-serializers library to provide extra serializers for Joda types. This PR bumps the version we depend on, and registers an additional serializer necessary for internal projects.

@pomadchin

pomadchin commented Sep 20, 2016 •

Copy link
Copy Markdown
Member

Have you checked it on a n node cluster (im curious how kryo behaves)?
// i believe these changes anyway would be a part of #1628

@fosskers

fosskers commented Sep 20, 2016 •

Copy link
Copy Markdown
Contributor Author

No, only locally. I'd need some help for the distributed case. What were the problems you were seeing before?

@pomadchin

pomadchin commented Sep 20, 2016 •

Copy link
Copy Markdown
Member

@fosskers spark replaces our kryo deps by its deps from its class path: #1286 please double check that replacement at least on a current lc emr demo (on emr cluster).

@fosskers

Copy link
Copy Markdown
Contributor Author

This PR will also die if we deprecate Java 7, since we can unregister all the Joda serializers.

@pomadchin

Copy link
Copy Markdown
Member

I don't like that "de.javakaffee" % "kryo-serializers" % "0.38" depends on kryo 3.0.3, as we exclude it and replace by spark compatible 2.21; but kryo 3.x binary incompatible with 2.x, that may cause problems (or may not, that would be great) on a real cluster.

@pomadchin pomadchin mentioned this pull request Sep 20, 2016
5 tasks done
@fosskers

Copy link
Copy Markdown
Contributor Author

This can be ignored, @pomadchin added the changes to his Spark 2 PR.

@fosskers fosskers closed this Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants