twitter / storehaus

Storehaus is a library that makes it easy to work with asynchronous key value stores
Apache License 2.0
465 stars 92 forks source link

storehaus-ostrich #129

Open sritchie opened 11 years ago

sritchie commented 11 years ago

This module should provide a Storehaus combinator for https://github.com/twitter/ostrich. I'd like to see latencies and success and failure rates reported on the future calls in get, put and merge (and their multi counterparts).

ryanlecompte commented 11 years ago

I thought ostrich was in the process of getting deprecated at twitter?

On Jul 24, 2013, at 8:55 PM, Sam Ritchie notifications@github.com wrote:

This module should provide a Storehaus combinator for https://github.com/twitter/ostrich. I'd like to see latencies and success and failure rates reported on the future calls in get, put and merge (and their multi counterparts).

— Reply to this email directly or view it on GitHub.

sritchie commented 11 years ago

Hmm, not that I know of. link?

ryanlecompte commented 11 years ago

@stevegury can comment - isn't ostrich supposed to be getting deprecated in favor of a new twitter stats library?

softprops commented 11 years ago

@sritchie You've got a big company :) I think this looks there's a migration path to using this library those it's much less ( not at all :) documented then Ostrich. It's a little confusing because there's also this library , also named metrics. Everybody loves metrics! Including me.

I'm super interesting in taking a stab at this one if you'd let me, but if you already started this I had a few suggestions.

1) don't tie this to a metics library. It's a very cool thing that storehaus makes a best attempt at minimally coupling itself to everything else in the twitter engineering ecosystems. When you do that you give companies that would use this but already maintain their own metrics infrastructure one less reason to use this library. It would be forcing another library that does the same thing into a codebase.

2) expose an interface for an underlying metrics library to do its thing. That way I can I plugin in my own metrics library. I'd call this module storehaus-instrument ala java.lang.instrument.*

3) then you can provide a module or modules that fill in the instrumentation interface, i.e. storehaus-ostrich ( or storehaus-twitter-metrics ), storehaus-codahale-metrics, storehaus-statsd, yada yada. This gives storehaus an extension point for plugable instrumentation, a bonus for everyone.

4) make instrumentation optional. I think you covered this by mentioning the combinator. Something like store.instrument(instrumentor) where instrumentor is an implementation of the interface defined under the storehaus-instrument module.

Again, I'd be super happy to flush this out with plenty of code review of corse. Let me know.

softprops commented 11 years ago

Just jotting down some ideas before I forget them. I like a laundry list of what kinds of interesting instrumentation information users would find useful. Something like finagles is pretty well defined.

Thinking outside the context of finagle, what would the laundry list be for storehaus?

stealing from from the readme

definitely present,
definitely missing, or
unknown due to some error (perhaps a timeout, or a downed host).

where op is one of get, multi-get, put, multi-put, merge, ...

what else?

stevegury commented 11 years ago

@ryanlecompte @sritchie Yes, I confirm that we'll deprecate ostrich in favor of twitter-server. We are currently in the process of migrating/testing a few services for learning from this experience.

As you already depend on finagle, I recommend that you use the StatsReceiver abstraction provided by finagle-core, then you're not coupled to any stats library.

sritchie commented 11 years ago

exactly what I was thinking, @softprops. this is good stuff.

softprops commented 11 years ago

@stevegury storehaus core doesn't depend in finagle yet. A few modules depend on finagle clients.

fingale's embedded stats library library looks clean. It also looks like it had little do with with finagle itself. Looks like some cross over with other metrics libraries. The same is true with the new twttr metrics library and Ostrich itself. Sometimes I wish there were a stats interface not tied to an implementation that all of these implementations implemented!

Since there's not an interface on hand. I wanted to suggest storehaus might define want which can be filled in with a specific implementation in a separate module. The nice thing about interfaces and implementations is that when a new shiny metrics library comes along you don't have to rewrite everything to use that.

On the other hand, I don't like duplicating effort. So I'm up for debate. I don't think storehaus, an library abstraction working for asynchronous key value stores, needs to depend on finagle, a library abstraction for building up netty pipelines for various protocol codecs. The two domains seem orthogonal.

stevegury commented 11 years ago

Sorry for the oversight, I was thinking that storehaus was depending on finagle. I agree that the best thing to do is decoupling the storehaus from the stats library, the abstraction that we're using in Finagle StatsReceiver is a good fit for us, I would recommend to use something similar.

sritchie commented 11 years ago

I do wish that StatsReceiver could be pulled out into a separate jar. @stevegury, what do you think?

softprops commented 11 years ago

There's something in there that do depend on finagle. https://github.com/twitter/finagle/blob/master/finagle-core/src/main/scala/com/twitter/finagle/stats/LoadedStatsReceiver.scala#L3 for instance. It would be cool for just the stats interface and nothing else were pulled out.

softprops commented 11 years ago

In case any one hasn't already I started working on some thoughts I had over here I'm doing "scala-doc driven development". I'm keeping track of my ideas here. I of like the idea of using package objects for a put down the core ideas behind the package. At least so there's on place I can keep the sketch in my head while flushing out the details.

softprops commented 11 years ago

I started implementing Instrumentation based on the rough design of finagles StatsReceiver. Since we already have the abstraction, I thought it would be interesting to implement one path of instrumentation in terms of MutableCaches. Storehaus instrumenting storehaus in terms of storehaus. How about that! I made a dummy in memory impl to test with so I can test my design.

https://gist.github.com/softprops/6352786

I'm going to be out of town in Seoul for the next few weeks so I will probably lose time to work on this during that time. In the meantime feedback is welcome.

sritchie commented 11 years ago

Hey, seeing this now. I'll give this a review today. Awesome that you're working on this!

sritchie commented 11 years ago

Man, this is looking really good. I like the proxy pattern... so often we wrap a store and manually delegate all methods over. As long as we can be careful with the trait stacking, I like this.

I think that this will be extremely useful. Definitely advanced enough for pull req status.

softprops commented 11 years ago

Yea I'm still thinking about the best way to handle the proxy issue this without making a mistake. I'm not sure this is the right one. I really like the enrichment pattern storehaus uses though. I borrowed heavily from that design here. I'm not sure where the best place is to put the implicit conversions. They are in the package object now. I sometimes feel like that's a bad pattern.