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

Support for creating a store-service for Storm platform #563

Merged
merged 9 commits into from
Dec 19, 2014

Conversation

egonina
Copy link
Contributor

@egonina egonina commented Dec 3, 2014

To be able to do leftJoin on a store we need the same object to serve as a store and a service. This is already supported in the Scalding Platform. This adds a trait to the Storm Platform that is of both P#Store and P#Service types. This can be used in jobs that use leftJoin against store. Users in their jobs can call:

Storm.storeService(mergeableStore, serviceStore)(batcher)

where the mergeableStore is the store for aggregation (delta store) and serviceStore is the store that we use as a service (can be delta store or the total store). For read-modify-write behavior the serviceStore should include the delta store, i.e. serviceStore can be a client store that wraps the delta mergeableStore or a client store that wraps the delta mergeableStore and the offline store (i.e. the total store).

new CombinedServiceStoreFactory[K, V] {
def mergeableStore = mStore
def mergeableBatcher = b
def serviceStore = () => ReadableStore.empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a sane constructor to have? left join would always come back None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, yea, we should only have the constructor with both stores defined

@@ -25,5 +25,5 @@ import com.twitter.storehaus.ReadableStore

case class ReadableServiceFactory[-K, +V](
store: () => ReadableStore[K, V]) extends OnlineServiceFactory[K, V] {
def create = store()
def serviceStore = store
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you write a return type on this. It is ambiguous (are we applying the function or no?).

Also, all public methods should have types.

@@ -25,5 +25,5 @@ import com.twitter.storehaus.ReadableStore

case class ReadableServiceFactory[-K, +V](
store: () => ReadableStore[K, V]) extends OnlineServiceFactory[K, V] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can just be:

case class ReadableServiceFactory[-K, +V](override val serviceStore: () => ReadableStore[K, V]) extends OnlineServiceFactory[K, V]

Copy link
Collaborator

Choose a reason for hiding this comment

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

no love for my simplification. :) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't see this note :) will add the simplification

@johnynek
Copy link
Collaborator

I think this is almost ready to merge, I just want to see the use case as easy as possible to do, and come with very clear comments.

I think it is easy to get confused trying to use this currently (the wiring up of the clientstore, why it is needed, etc...)

@egonina
Copy link
Contributor Author

egonina commented Dec 16, 2014

Thanks for the comments @johnynek. I moved the ClientStore stuff to CombinedServiceStoreFactory and added comments.

Also, changed ClientStore to use Semigroup instead of Monoid, which fixes #569

@egonina
Copy link
Contributor Author

egonina commented Dec 17, 2014

@johnynek this is ready for merge

ianoc added a commit that referenced this pull request Dec 19, 2014
Support for creating a store-service for Storm platform
@ianoc ianoc merged commit 8f6e805 into develop Dec 19, 2014
@ianoc ianoc deleted the kgonina/store_service_storm branch December 19, 2014 19:12
@johnynek
Copy link
Collaborator

Nice work here!

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 this pull request may close these issues.

3 participants