Fix serialization of KryoHadoop

[ttim]

In process of Summingbird migration I found that serialization of KryoHadoop class was broken recently, this PR adds test for that and fixes it.

[johnynek]

Lgtm. We can also add a serializer for Config by just converting to and from a Map[String, String] I think.

[ttim]

@johnynek Do you mean to keep Config serializable instead and therefore there will be no need to have parts of it as vals?

I guess it was more or less this way before, but after @ianoc changed behavior to what we see now 2c72bac.

[johnynek]

I think we should make your change AND make Config Kryo serializable. Belt and suspenders.

[ttim]

Now I see. It's not simple (at least in this PR) for now because Config doesn't expose a way to iterate over keys, so it's impossible to convert it into Map[String, String].

[dieu]

@ttim look on trait Config in scalding, it has methodtoMap

[ttim]

@dieu small clarification - what we pass there isn't scalding config but chill's one, which doesn't contain this method.

[johnynek]

ahh yes. We should open an issue on Chill to make all of the Config Serializable and Kryo serializable.

[ttim]

@johnynek I'm wondering how was it working before @ianoc change (2c72bac)? For me it looks like Config is serializable already (or maybe HadoopConfig, I don't know)?

[dieu]

@ttim everything is serializable in chill.Config, but only by kryo I think, because chill.Config is not marked as serializable.

[ttim]

@johnynek So I guess we don't need to create an issue in chill because Config is already kryo serializable.

Also I did a small refactoring to make logic clearer (and avoid transient fields), what do you think?

[johnynek]

I'm not crazy about this at all. Multiple constructors is non-idiomatic in scala and many folks subclass this as it is. I'd rather do the first fix personally.

[johnynek]

To put it another way: this type, and hose like them, are created with reflection and expected to have a constructor that accepts a Config. This is part of chill-hadoop.

Also, if we add another option we would be adding another parameter to the constructor vs just reading more from the Config.

[ttim]

@johnynek my main motivation was to make object's state more clear (and I guess transient plays against it) and do separation between internal state and how do we create KryoHadoop clearer.
At the same time I don't like two constructors either and missed a point of creation by reflection, so I'm happy to revert this back.

[pankajroark]

@johnynek Do you think this PR is ready to go? We need this fix for upgrading summingbird to 2.12 twitter/summingbird#721. Thanks

[johnynek]

👍