-
Notifications
You must be signed in to change notification settings - Fork 155
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 chill-algebird project by copying AlgebirdSerializers from of scaldi... #177
Conversation
} | ||
} | ||
|
||
// class SpaceSaverSerializer[T] extends KSerializer[SpaceSaver[T]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, until this is in algebird release:
twitter/algebird#289
On Fri, Mar 14, 2014 at 1:26 PM, P. Oscar Boykin
notifications@github.comwrote:
In
chill-algebird/src/main/scala/com/twitter/chill/algebird/AlgebirdSerializers.scala:
- }
+}
+class HLLMonoidSerializer extends KSerializer[HyperLogLogMonoid] {
- setImmutable(true)
- val hllMonoids = MMapInt,HyperLogLogMonoid
- def write(kser: Kryo, out : Output, mon : HyperLogLogMonoid) {
- out.writeInt(mon.bits, true)
- }
- def read(kser : Kryo, in : Input, cls : Class[HyperLogLogMonoid]) : HyperLogLogMonoid = {
- val bits = in.readInt(true)
- hllMonoids.getOrElseUpdate(bits, new HyperLogLogMonoid(bits))
- }
+}
+// class SpaceSaverSerializer[T] extends KSerializer[SpaceSaver[T]] {
does this need to be commented?
Reply to this email directly or view it on GitHubhttps://github.com//pull/177/files#r10617565
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was merged
Can you add an AlgebirdRegistrar that extends IKryoRegistrar and registers all these serializers? |
Do we really not have any tests for this? I realize it's a code copy, but surprised we don't even have a round-trip... |
algebird 0.6.0 is out. Care to update and get things uncommented? Agree with @jcoveney that we should at least have some kind of tests here (check that the created registrar really has the right classes registered, that we can roundtrip items correctly). |
closes #176 |
ok i will add some tests and address the commented stuff On Sat, Apr 19, 2014 at 7:42 PM, P. Oscar Boykin
|
add chill-algebird project by copying AlgebirdSerializers from of scaldi...
...ng