Skip to content
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

Refs #35: Custom generic serialisation via type class #86

Merged
merged 8 commits into from
Apr 3, 2016

Conversation

lloydmeta
Copy link
Contributor

Just a prototype of what I mentioned on the issue, to serve as a point of discussion and garner feedback.

Goals:

  • Provides sensible serialisation for primitive types out of the box
  • If desired, users can import from an object housing a Java-serialising object serialiser
  • If end users have their own object-to-MsgPack/JSON/whatever codecs, they just need to provide a suitable auto converter to our Codec (conversely, this project can have projects that provide bindings for commonly used formats)

Changes:

  • Add Codec typeclass to allow adhoc customisation of serialisation/deserialisation
  • Add some default Codecs for standard primitives w/ tests
  • Add a Java-basd object serialiser w/ test
  • Modify various existing methods, macros, and tests to make this work

Misc:

  • Bump PGP plugin version

- Add Codec typeclass to allow adhoc customisation of serialisation/deserialisation
- Add some default Codecs for standard primitives w/ tests
- Add a Java-basd object serialiser w/ test
- Modify various existing methods, macros, and tests to make this work

Misc:
- Bump PGP plugin version
@cb372
Copy link
Owner

cb372 commented Mar 28, 2016

Sorry for the slow review.

I just spent ages writing a huge, detailed comment but it got deleted. Let's see if I can summon the willpower to write it all out again. The sacrifices we make for FOSS...

I agree that a typeclass is a good way to implement custom serialisation. I've experimented with it in the past, but never quite got around to implementing it properly. This PR looks like a great start. Just a couple of general comments before I go through the code and make a few OCD-driven requests for changes.

Compatibility

Changing the serialisation format is a big breaking change for Memcached and Redis users. After upgrading ScalaCache their cache will be effectively cold and their logs will probably start filling up with warnings about deserialisation failures.

In one of my previous experiments with custom serialisation, I tried implementing a Codec that emulated the behaviour of the default Spymemcached Transcoder in order to preserve compatibility. But it turned out to be quite tricky because the data would have to pass through both our codec and Spy's transcoder, in effect becoming double-serialised.

I think a more pragmatic solution would be to add a boolean flag to MemcachedCache and RedisCache that disables the codec-based serialisation and uses the existing implementation. This would at least let people decide if and when they want to take the hit of the serialisation format change. This flag would also need to be clearly documented in the README and the changelog.

Compression

Spymemcached's default transcoder automatically gzips anything over 16KB. Might be nice to do that in ScalaCache as well, at least as an option. But it does complicate things, as you then need to write a header containing a compression flag.

Import tax

I'd like to keep imports to a minimum. Ideally just writing import scalacache._ should bring the Java serialisation codecs into scope as low priority implicits. I think we can achieve this by making the scalacache package object extend from JavaSerializationCodecs. Can a package object extend from a trait? Not sure, probably yes.

@@ -1,5 +1,6 @@
package scalacache.caffeine

import scalacache.serdes.Codec
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we make the package scalacache.serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool done.

@lloydmeta
Copy link
Contributor Author

I just spent ages writing a huge, detailed comment but it got deleted. Let's see if I can summon the willpower to write it all out again. The sacrifices we make for FOSS...

😢 I hate it when that happens. Thanks for rewriting your feedback; I know it isn't easy.

Compatibility

Good point; I've added a useLegacySerialization parameter to the Memcached and Redis cache implementations that bypasses usage of the Codec when set to true.

Compression

I think that is a great idea. Would it be ok with you if that were to be added on in another PR ? Since we would be compressing and decompressing bytes, it seems reasonable to treat compression as a separate concern.

Import tax

I hear you on the wanting to lower the amount of things that people need to import, but scalacache._ seems like the main bootstrapping import for reasonably kosher things that are relatively stable.

Mixing in JavaSerializationCodecs into that package object would mean that Java-serialisation-based Codec would be available by default. Since Java serialisation is really slow, I think it might be better to have it be available only after paying a small import tax, as a warning to developers using the lib. It seems better to save scalacache._ for when we come up with some really nice and fast generic Codec derivation-magic so that we can avoid deserialisation problems at that point in time. It also saves us from having an Any Codec in-scope inside that package, which is useful in case we forget to declare implicit params when adding/modifying the code.

Also, when using generic typeclasses (whether it's deriving them or bringing things in scope that derive them), I think people are familiar with having to do an additional import; Play, Argonaut, Circe etc have somewhat established this pattern.

- Add a `useLegacySerialization` param to Memcached and Redis-related caches that activates old
  behaviour
- Add explanation of Codec and Legacy Serialization option to readme and Changelog

- Add tests for legacy codec support
@lloydmeta lloydmeta force-pushed the feature/custom-serialization branch from c855f66 to ed20a0e Compare March 29, 2016 16:20
@cb372
Copy link
Owner

cb372 commented Apr 2, 2016

Compression

Yep, let's add it in another PR. Could even be a separate codec implementation.

Import tax

I think Java serialisation is a sensible default, even if it is slow. It's good enough for a lot of use cases (and hasn't put many people off using ScalaCache until now). Also, low barrier to entry is a high priority for me - a lot of people don't want to care how their stuff is being serialised, they just want to cache stuff as easily as possible.

And don't forget about people using Guava/Caffeine/Ehcache/Lru. They would have to import a serialisation codec that they won't even use, which is both annoying and confusing.

@lloydmeta
Copy link
Contributor Author

Import tax

Ok got it. I think your reasoning is sound. Done in a424e52

Fix implicit not found message
@lloydmeta lloydmeta force-pushed the feature/custom-serialization branch from a424e52 to 6963d8d Compare April 3, 2016 04:29

New features:

* [#86](https://github.com/cb372/scalacache/pull/86) adds support for custom serialisation. Thanks to @lloydmeta
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, thanking yourself in the changelog. Nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is some deep existential stuff right there.

Or just blind copy pasting of existing stuff. 😆

@cb372 cb372 merged commit 07282f6 into cb372:master Apr 3, 2016
@lloydmeta lloydmeta deleted the feature/custom-serialization branch April 3, 2016 11:42
@lloydmeta lloydmeta mentioned this pull request Apr 4, 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.

None yet

2 participants