I reduced the error hiding in execute, so hopefully that addresses (2). As for (3), I created a module called riakpool_client which wraps the calls to execute in a more familiar way. Use of that module is completely optional. At the minimum it can serve as a nice set of examples for application-specific implementations that the user may wish to write.
Dave On Tue, Apr 12, 2011 at 8:49 PM, David Weldon <[email protected]> wrote: > Thanks for taking the time to review the code Ryan. All three of those > issues have been bothering me. > > 1) I haven't been very concerned about growing the pool in case of a > spike since client creation isn't particularly expensive... at least > my tests didn't indicate that was a cause for concern. The fact that > the pool never shrinks does bother me though. Since the connections > rotate in a circular buffer there is unlikely to be a case where they > just go unused. I think the only way to deal with the problem would be > some kind of heuristic, which implies application-specific knowledge. > I'm open to suggestions if someone has an elegant solution. > > 2) I can't let execute/1 throw an exception because then the check-in > won't happen. Maybe the code should do something like: catch error:E > -> {error, E}. I'm cool with having the function return {ok, any()} | > {error, any()}. > > 3) I find this the most objectionable part of the app, but I don't > have a good answer. Because I have no idea what the user is going to > do with his/her Fun, I can't make any guarantees about the return > values without wrapping them. If someone can point out a flaw in this > logic I'm psyched to change the code. > > Dave > > On Tue, Apr 12, 2011 at 7:38 PM, Ryan Zezeski <[email protected]> wrote: >> Dave, >> First off, I want to say that I think it's awesome that you've started a >> riakc pool implementation and shared it with everyone! It's certainly a >> missing piece and we could all benefit from having a canonical >> implementation. That said, I have a few comments: >> 1) Growth of the pool: Since the pool has no ceiling things could get out of >> control under the right conditions. Just as important, I don't see any >> functionality to shrink the pool when things are quiescent (default behavior >> of riakc is to not timeout the conn). Also, you may want to grow by more >> than 1 to handle spikes better. >> 2) Concealing errors: I like how you made a HOF (execute) to deal with conn >> checkin/checkout but I don't like how it covers up the cause of an error, I >> would let it escape. E.g. if someone fat fingers the module name they will >> get 'error' back instead of the much more helpful 'undefined ...'. >> 3) Wrapping the return value: You wrap the return val like `{ok,F(C)}`. I'd >> just return the value of F(C) so existing clients don't have to change their >> patterns. Plus `{ok,{ok,Obj}}` feels funny. >> Anyways, these are just some thoughts after browsing the code a little. I'm >> no expert in building conn pools--not even a beginner for that matter. I >> think it's a good start and I hope you continue working on it and improving >> it. I may even send some pull requests your way in the future. >> Please keep us updated on your use of Riak in production. It's always nice >> to hear :) >> Thanks, >> -Ryan >> P.S. I've been wanting to checkout mailrank's Haskell pool. Those darn >> Haskell folks always seem to build good stuff. > _______________________________________________ riak-users mailing list [email protected] http://lists.basho.com/mailman/listinfo/riak-users_lists.basho.com
