SPARK-1813. Add a utility to SparkConf that makes using Kryo really easy

[sryza]

No description provided.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15022/

[mridulm]

As I mentioned in the jira, I don't see value in this change - it is a corner case trying to save about 5 lines of straightforward code while adding to the public api

[sryza]

Do you mind elaborating on why it's a corner case? My understanding is that Kryo is the right choice over Java serialization in 99% of cases, and any time somebody wants to use their own objects with Kryo serialization, they need something like those 8 lines of code. I've had the opportunity to teach a bunch of people how to use Spark over the last few months and how to write those lines of code has come up almost every time.

[mridulm]

I have elaborated in the jira, but I will repeat it again for clarity:

This is adding an api for a specific case - it assumes single serialization
type (kryoserializable).
The reduction in boilerplate is not high, neither is it doing so for
something tricky/implementation detail.

Particularly given stress of simplifying our api this is going the opposite
direction without sufficient value to justify it.
On 17-May-2014 4:34 am, "Sandy Ryza" [email protected] wrote:

Do you mind elaborating on why it's a corner case? My understanding is
that Kryo is the right choice over Java serialization in 99% of cases, and
any time somebody wants to use their own objects with Kryo serialization,
they need something like those 8 lines of code. I've had the opportunity to
teach a bunch of people how to use Spark over the last few months and how
to write those lines of code has come up almost every time.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/789#issuecomment-43388406
.

[ash211]

Alternative to adding to SparkContext's API, how about an API along the lines of:

Seq[Class[Any]] classes = ...
KryoRegistrator.enableKryoWithClasses(sc, classes)

The enableKryoWithClasses call would basically do the same thing as before but as a helper function.

[mateiz]

I like the idea of moving this into KryoSerializer or KryoRegistrator instead of SparkConf. Maybe it can be KryoSerializer.configure(conf, classes). In addition, this will need a Java-friendly API too. The easiest way is to make it take Array[Class[_]] though you have to see how that translates into Java. Please add unit tests in both Scala and Java so we can see the API being called there.

[mateiz]

BTW one other thing I don't fully like about this is that it's not modular -- you need to have a single list of all your classes in one place, and you can't for example have libraries that register their own classes in sequence. I'd prefer for us to think about this a bit more and maybe come up with an API that enables that. For example, imagine the following:

• The user has their own classes
• The user also calls MLlib, which has some classes it wants to register
• The user also calls GraphX, which has other classes it wants to register

It would be interesting to come up with a way where all these guys register their classes in some order. For now the easiest way is for the user's KryoRegistrator to call KryoRegistrators from these other libraries inside itself.

[sryza]

Putting this in KryoRegistrator/Serializer makes sense to me.

That's a good point about the modularity.

One way to deal with it would be to add a config property that carries classes to register. And to have the default KryoRegistrator register all the classes in this property. So the user/library-facing API could be KryoSerializer.registerClasses(SparkConf, Array[Class[_]]), which would append to this property.

Though would there be a good location for a library to call that API? Alternatively, libraries could expose a getClassesToRegister API. So if I'm using MLLib and GraphX, I call KryoSerializer.configure(conf, Arrays.concatenate(myClasses, MLLibKryo.getClassesToRegister, GraphXKryo.getClassesToRegister)). And each library can include its dependencies in its list.

[mridulm]

This is what we do currently.
Each module has its own registrator - and they are invoked in the order in
which they are registered to our uber registerator (since spark supports
only a single registrator) - which is what is specified as kyro
registrators.

On Tue, May 27, 2014 at 11:44 AM, Matei Zaharia [email protected]:

BTW one other thing I don't fully like about this is that it's not modular
-- you need to have a single list of all your classes in one place, and you
can't for example have libraries that register their own classes in
sequence. I'd prefer for us to think about this a bit more and maybe come
up with an API that enables that. For example, imagine the following:

• The user has their own classes
• The user also calls MLlib, which has some classes it wants to
  register
• The user also calls GraphX, which has other classes it wants to
  register

It would be interesting to come up with a way where all these guys
register their classes in some order. For now the easiest way is for the
user's KryoRegistrator to call KryoRegistrators from these other libraries
inside itself.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/789#issuecomment-44239090
.

[mateiz]

@sryza are you still looking at coming up with a more modular version of this? If not, it would be good to close the PR for now.

[sryza]

@mateiz I posted a couple ideas and was waiting on feedback. Any thoughts?

[mateiz]

Ah, I see. In that case I'd prefer something like this:

MLlibUtils.registerKryoClasses(conf)
GraphXUtils.registerKryoClasses(conf)
conf.registerKryoClasses(Array(/* your classes */)

The one tricky bit is if two libraries use the same classes. Maybe we can keep track of which classes are registered somehow, otherwise Kryo might get confused.

[sryza]

Updated patch adds the APIs discussed. It relies on a new property spark.kryo.classesToRegister, which registerKryoClasses appends to. The change also enables users to register classes entirely through configuration, by setting this property directly.

[SparkQA]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20764/

[andrewor14]

retest this please

[andrewor14]

Does the order of registering these classes matter? Here we're not preserving the order set through spark.kryo.classesToRegister, but maybe that's not important.

[sryza]

Here's a thread that indicates order is important, at least in the sense that the kryo instance serializing should have the same order as the instance deserializer:
https://groups.google.com/forum/#!topic/kryo-users/E-0_EVi-O1Q

String's hash code is deterministic, so it shouldn't really matter, but I think it doesn't hurt to be conservative.

The LinkedHashSet should preserve order, unless I'm missing something?

[andrewor14]

Ah never mind, it's a LinkedHashSet. I always trip over these java.util.LinkedHash* collections and think they're just hash collections that don't maintain ordering.

[SparkQA]

QA tests have started for PR 789 at commit 22bbeec.

• This patch merges cleanly.

[andrewor14]

Can you use this style for easier readability

val reg = conf.getOption(...)
  .map(...)
  .getOrElse(...)

[andrewor14]

Hey @sryza this looks good. Creating a custom class is cumbersome if the user is just trying things out, and now it becomes just 1 extra line to do it. Will the new API work in Java? Also, spark.kryo.registrator is still documented in the list of configurations. Maybe we should replace this with an entry for spark.kryo.classesToRegister on this list as well in addition to showing how the user can add them dynamically as you have done.

[SparkQA]

QA tests have finished for PR 789 at commit 22bbeec.

• This patch fails unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20814/

[sryza]

That's correct (documented this on the conf page). My thought was that we could hit strange interactions, for example if the same class is registered both with a custom registrator and with the classes given through registerKryoClasses. I can add this in if you think having both would be useful.

[mateiz]

I think it's better to do both and explain that there might be problems. Otherwise users will see this new API and perhaps be surprised that their old registrator is no longer called. Not everyone reads the docs on the new API, so they might never notice, and just get poor performance.

BTW looking at Kryo's docs, it does support multiple register calls on the same class, and it just uses the value from the last one. So it will probably do the right thing here if we call their custom registrator last.

[sryza]

Updated patch allows using both at the same time

[SparkQA]

QA tests have started for PR 789 at commit 1be3fa5.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 789 at commit 1be3fa5.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • throw new SparkException("Failed to load class to register with Kryo", e)

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21175/

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21182/

[sryza]

Jenkins, retest this please

[SparkQA]

QA tests have started for PR 789 at commit 2def654.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 789 at commit 2def654.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • throw new SparkException("Failed to load class to register with Kryo", e)

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21185/

[mateiz]

Thanks, this looks good to me now, though there are some merge conflicts. Mind rebasing it?

[SparkQA]

QA tests have started for PR 789 at commit 48b05e9.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 789 at commit 48b05e9.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • throw new SparkException("Failed to load class to register with Kryo", e)

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21726/
Test PASSed.

[pwendell]

Thanks Sandy - I've merged this!