Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Storm Store type should take Semigroup[V] #677

Closed
johnynek opened this issue Jul 21, 2016 · 1 comment · Fixed by #687
Closed

Storm Store type should take Semigroup[V] #677

johnynek opened this issue Jul 21, 2016 · 1 comment · Fixed by #687

Comments

@johnynek
Copy link
Collaborator

Right now, the producer graph has a Semigroup[V] in sumByKey, but actually, the storm platform ignores it.

Scalding does not.

https://github.com/twitter/summingbird/blob/develop/summingbird-online/src/main/scala/com/twitter/summingbird/online/MergeableStoreFactory.scala

A fix for this is to have MergeableStoreFactory really be something like Semigroup[V] => Mergeable[K, V] rather than () => Mergeable[K, V].

We can be somewhat backwards compatible by giving the old methods (but marking them as deprecated) and just ignoring the Semigroup passed in.

Without something like this #562 is pretty dangerous because online is would be really easy to have a mismatch between the store semigroup and the aggregator semigroup (this is actually already pretty dangerous and not clear how we didn't address this before now).

@pankajroark
Copy link
Contributor

I agree, it sounds like a good idea to supply semigroup used during producer graph creation to users for creating the Mergeable. It does reduce risk of users using a different semigroup even though it doesn't eliminate it completely. With proper documentation we can reduce the chances of making a mistake.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants