Added storehaus-hbase #139
Conversation
|
|
||
| import org.apache.hadoop.hbase.client.{Delete, Put, Result, HTablePool} | ||
| import com.twitter.storehaus.Store | ||
| import org.apache.commons.lang.StringUtils._ |
There was a problem hiding this comment.
We probably want to limit the scope of this import to the place(s) where it's used.
|
@ryanlecompte Thanks for the feedback, I have incorporated most of them. Please take a look @johnynek Oscar, what do you want me to do about the test. They are failing since CI env doesnt have HBase install and there is no easy way to mock out HBase Thanks |
|
In the before script, before_script part of the .travis.yml: Travis CI doesn't have hbase preinstalled yet, but you can probably 'sudo apt-get install hbase' and set things up with a couple more commands. It will take a bit longer build but hopefully by not too much. |
|
That'd be fantastic to try and really helpful, if it's not too much trouble. |
|
I was able to mock HBase using HBaseTestingUtil and the test is finally passing. There are some issues with running HBase natively on Travis and once they are resolved I will switch the test to use HBase service( travis-ci/travis-cookbooks#40). HBaseTestingUtil is kind of flaky and gets into an unstable state if you have multiple test using the minicluster. I spent sometime and couldnt get to work with multiple tests, so we have one CI test right now. Again, this will go away once HBase is supported by travis @sritchie Let me know if its good enough to merge |
|
|
||
| def putValue[K, V](kv: (K, Option[V]))(implicit keyInj: Injection[K, Array[Byte]], valueInj: Injection[V, Array[Byte]]): Future[Unit] = { | ||
| kv match { | ||
| case (k, Some(v)) => Future { |
There was a problem hiding this comment.
worried about Future.apply here... this does call-by-name, so it actually won't start processing until the user does anything. I think we need to make a FuturePool and create a future that way, so that this computation becomes async BUT doesn't block. Thoughts?
|
Made one comment - other than that, this looks awesome. Let me get your thoughts on that and then let's fix it or just merge. |
|
@sritchie Good call on FuturePool. I have modified the code, please take a look |
|
Looks great! Pulling in now. We'll release this on the next push. |
Had to upgrade bijections to 0.5.3 as I needed bijections-hbase. Also note that tests require a working HBase setup and will probably fail the CI build. Not sure how you want to handle it, I can ignore the test if you like