LevelDB support #258
LevelDB support #258
Conversation
…ls with native libraries
| * store fails to return a value for a given key, that should be represented | ||
| * by a Future.exception. | ||
| */ | ||
| override def multiGet[K1 <: Array[Byte]](ks: Set[K1]) |
There was a problem hiding this comment.
This is defined in the parent class so not needed here right? Likewise for multiPut and close(time: Time)
There was a problem hiding this comment.
Ah, please disregard. Just saw your PR comment about implementing this.
|
@BenFradet Thanks for the PR. Mostly minor comments on this version. |
|
Thanks a lot for your feedback, will take it into account. |
|
All unit tests for the module are passing. |
| * @author Ben Fradet | ||
| * @since 10/03/15 | ||
| */ | ||
| class LevelDBStore(val dir: File, val options: Options, val numThreads: Int) |
There was a problem hiding this comment.
numThreads can be defaulted to Runtime.getRuntime.availableProcessors perhaps. Some of the other store wrappers that use future pool do this.
| class LevelDBStore(val dir: File, val options: Options, val numThreads: Int) | ||
| extends Store[Array[Byte], Array[Byte]] { | ||
|
|
||
| private lazy val db = factory.open(dir, options) |
There was a problem hiding this comment.
is this thread-safe? Can you link to docs showing this? If not, we should use:
https://github.com/twitter/util/blob/master/util-core/src/main/scala/com/twitter/concurrent/AsyncMutex.scala
Then we'd do something like:
dbMutex.acquireAndRun(futurePool {
// method logic here.
})There was a problem hiding this comment.
Will look into it and post my findings.
|
Thanks for the feedback, will take it into account. |
… the number of available processors
| // http://stackoverflow.com/questions/19425613/unsatisfiedlinkerror-with-native-library-under-sbt | ||
| testOptions in Test := Seq(), | ||
| fork in Test := true | ||
| ).dependsOn(storehausAlgebra % "test->test;compile->compile") |
There was a problem hiding this comment.
This can be just storehausCore?
There was a problem hiding this comment.
Yup, I think that's what I meant to do when I did this.
|
I haven't looked into whether private lazy val db = factory.open(dir, options)was thread-safe or not yet. |
|
My bad, it seems to be thread-safe after all (cf this). |
| * It is undefined what happens on get/multiGet after close | ||
| */ | ||
| override def close(time: Time): Future[Unit] = { | ||
| futurePool { db.close() } |
There was a problem hiding this comment.
this should be: futurePool { db.close() }.flatMap { _ => super.close(time) }
|
one minor issue, and remerge with develop and it should be good. Thanks. |
|
Will do. |
|
Travis fails due to an ElasticSearch test. |
|
Restarted the build. |
|
I'll merge develop in order to take advantage of #267 soon. |
|
@BenFradet Sorry, looks like we missed this one for the ongoing minor version release. Yes let's merge once this is green and we can add it to the next release. |
|
@rubanm yup, no problem. Hopefully, it'll turn green this time. |
|
Merging, thanks! |
As per #51, this is the first draft for supporting LevelDB inside storehaus, feedback is more than welcome.
I'm planning on doing
multiGetandmultiPutin the coming week.I intend to do the same thing with RocksDB once I'm done here.