Conversation
tyvsmith
commented
Jul 14, 2025
Co-authored-by: tys <tys@uber.com>
|
|
|
|
||
| override suspend fun put(key: String, value: ByteArray?): ByteArray { | ||
| requireOpen() | ||
| return withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
If I'm reading this correctly, in the original Java impl, every get, put, contains call happens on a single-threaded executor. That executor guarantees that tasks are processed FIFO one at a time. The Kotlin impl is moving work off of that single queue to a shared Dispatchers.IO pool. To maintain ordering and prevent races, I think we will need a dedicated single-threaded dispatcher (or sequential executor). I don't think a Mutex would be sufficient because overall ordering of operations would still be non deterministic
| */ | ||
| fun SimpleStore.getStringFuture(key: String): ListenableFuture<String> { | ||
| return kotlinx.coroutines.guava.asListenableFuture( | ||
| kotlinx.coroutines.GlobalScope.async { getString(key) } |
There was a problem hiding this comment.
Agree Java consumers shouldn't need to know anything about coroutines, but concerned about GlobalScope. In the Java version, lifecycle management is handled entirely inside the store impl. I think we could replicate this in Kotlin without exposing coroutines to Java callers by hiding a default scope inside the store impl. For example, defining an internal interface that exposes a scope, having our impl implement it, then having the extension cast
| import java.io.Closeable | ||
|
|
||
| /** Fast, reliable storage. */ | ||
| interface SimpleStore : Closeable { |
|
Would it be useful to break this into two PRs, one to convert to Kotlin but still use ListenableFuture? Moving straight to Kotlin coroutine scopes is risky since the original impl very purposely handled ordering internally. |