New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfigSerializer to PersistingContext #95

Merged
merged 4 commits into from Jun 19, 2018

Conversation

Projects
None yet
3 participants
@labianchin
Copy link
Contributor

labianchin commented Jun 18, 2018

I think I found an issue with flo, kryo and typesafe/lightbend Config serialization:

"ConfigObject is immutable, you can't call Map.put"
twitter/chill#265

The solution is to create and register a serializer for SimpleConfig class.
This PR adds the serializer on flo-freezer.
Still, I think it might be questionable to add a serializer here upstream. For Config it might generic and worth. But I wonder if later flo should allow users to register their own serializers.

@labianchin labianchin force-pushed the labianchin:master branch from 214e1ce to b3a3bb7 Jun 18, 2018

@labianchin labianchin force-pushed the labianchin:master branch from b3a3bb7 to 6786b94 Jun 18, 2018

@labianchin

This comment has been minimized.

Copy link
Contributor Author

labianchin commented Jun 18, 2018

kryo.register(Class.forName("com.typesafe.config.impl.SimpleConfig"), new ConfigSerializer());
} catch (ClassNotFoundException e) {
LOG.debug("ConfigSerializer not registered", e);
}
kryo.register(ClosureSerializer.Closure.class, new ClosureSerializer());
kryo.setInstantiatorStrategy(new Kryo.DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));

This comment has been minimized.

@honnix

honnix Jun 18, 2018

Contributor

It seems we don't set this for serializing. What does it achieve?

This comment has been minimized.

@labianchin

labianchin Jun 18, 2018

Author Contributor

I am not sure either. I am trying to deduplicate the code a bit. Should we go back having that part duplicated?

This comment has been minimized.

@honnix

honnix Jun 18, 2018

Contributor

We will take a look later to make sure we don't break anything.

This comment has been minimized.

@honnix

honnix Jun 18, 2018

Contributor

It doesn't seem to matter because it is only used for deserializing, just being weird. We might do that after https://github.com/spotify/flo/pull/95/files#diff-d7f1d9c331c8fb2c839cdd4ff6cb0a16R121 to make it less confusing.

This comment has been minimized.

@labianchin

labianchin Jun 19, 2018

Author Contributor

Sure. Shall I do the change?

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

Sure please go ahead. You own this PR. =D

This comment has been minimized.

@labianchin

labianchin Jun 19, 2018

Author Contributor

👍 Done.

} catch (IOException e) {
throw new RuntimeException(e);
}
}

public static void serialize(Object object, OutputStream outputStream) {

This comment has been minimized.

@honnix

honnix Jun 18, 2018

Contributor

Any reason we expose another two APIs?

This comment has been minimized.

@labianchin

labianchin Jun 18, 2018

Author Contributor

Yes. Then we can have unit tests for tasks, trying serialize and deserialize only using Streams and without using the file system.

These unit tests and/or calling FloRunner.runTask is important otherwise unserializeable flo tasks would only be noted at runtime in production.

This comment has been minimized.

@honnix

honnix Jun 18, 2018

Contributor

👍

try (Output output = new Output(newOutputStream(file, WRITE, CREATE_NEW))) {
kryo.writeClassAndObject(output, object);
}
serialize(object, Files.newOutputStream(file, WRITE, CREATE_NEW));

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

newOutputStream has been statically imported, the same for newInputStream.

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

@labianchin fix this? i will give +1 then. :)

This comment has been minimized.

@labianchin

labianchin Jun 19, 2018

Author Contributor

I removed the static imports. Or should I use them?

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

Ah, OK, either way. I didn't see that because it didn't close this comment. Sorry.

@@ -26,5 +26,9 @@
<groupId>com.twitter</groupId>
<artifactId>chill-java</artifactId>
</dependency>
<dependency>
<groupId>com.typesafe</groupId>

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

This should be optional otherwise user code will pull in this dependency anyway and that makes Class.forName() meaningless. Is that right? I mean in fact flo-runner depends on typesafe config anyway, right? So maybe no need for Class.forName().

This comment has been minimized.

@labianchin

labianchin Jun 19, 2018

Author Contributor

The reason for the Class.forName() is that SimpleConfig is a package private class, so I have not found any other way to reference it. I will add a comment about that.

About having the config dependency in flo-freezer/pom.xml, that is because the Config interface is references in the ConfigSerializer class.

This comment has been minimized.

@honnix

honnix Jun 19, 2018

Contributor

OK now I understood.

@labianchin labianchin force-pushed the labianchin:master branch from 4653d7f to f8fae11 Jun 19, 2018

@honnix

This comment has been minimized.

Copy link
Contributor

honnix commented Jun 19, 2018

👍

@honnix honnix merged commit 8e4cbcd into spotify:master Jun 19, 2018

@danielnorberg

This comment has been minimized.

I guess serialize() should not close the passed in outputStream as it does not own it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment