Reporting store algebra's #176
Conversation
|
|
||
| package com.twitter.storehaus.algebra.reporting | ||
|
|
||
| import com.twitter.algebird.Monoid |
|
@softprops you have an Proxy idea that seems like a slightly less structured Reporter. Pretty sure we don't need both of these ideas. I feel that the Reporter, with more fixed behavior has a clearer usage pattern: do something with the input and output of a store as a sideeffect. Seems like this could be used with your instrumentation to do counters. |
|
@johnynek Proxy is more of a utility make combinators that want to add some behavior to a store type without requiring the need to override all of thats store type's methods, the "wrapper" problem. If you mix this in you only need to implement the methods that are interesting for the new type you are defining. The rest will be covered by the proxy. The instrumented types are defined in terms of proxies because once you wrap a store, to appear as an instance of that store, you also need to implement ( and not forget ) to manually override and delegate to the wrapped store methods like |
Use proxies, reduce code a bit
| } | ||
| } | ||
|
|
||
| trait ReadableStoreReporter[K, V] extends ReadableStore[K, V] { |
There was a problem hiding this comment.
How about:
trait ReadableStoreReporter[S <: ReadableStore[K, V], K, V] extends ReadableStore[K, V] {
def self: S // don't lose the type when we wrap.
def close = self.close // don't forget to close.
}
| override def multiGet[K1 <: K](keys: Set[K1]) = Reporter.sideEffect(keys, self.multiGet(keys), traceMultiGet) | ||
|
|
||
| def traceMultiGet[K1 <: K](ks: Set[K1], request: Map[K1, Future[Option[V]]]): Map[K1, Future[Unit]] | ||
| def traceGet(k: K, request: Future[Option[V]]): Future[Unit] |
There was a problem hiding this comment.
Should the trace methods be protected?
There was a problem hiding this comment.
Yep, sounds about right. Added
|
|
||
| object Reporter { | ||
| def sideEffect[T, U](params: U, f: Future[T], sideEffect: (U, Future[T]) => Future[Unit]): Future[T] = | ||
| Future.join(f, sideEffect(params, f)).map(_._1) |
There was a problem hiding this comment.
I was a little confused at first as to why join instead of f map ...
But this makes it possible to perform the side effect either sequentially or in parallel I guess?
There was a problem hiding this comment.
its to effectively just separate the two, they are independent futures in this model that we select one from to return to the user
|
LGTM |
No description provided.