-
Notifications
You must be signed in to change notification settings - Fork 16
Migrate simple-store to kotlin and coroutines #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Migrate simple-store to kotlin and coroutines #108
Conversation
tyvsmith
commented
Jul 14, 2025
Co-authored-by: tys <[email protected]>
|
|
matt-ramotar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
|
|
||
| override suspend fun put(key: String, value: ByteArray?): ByteArray { | ||
| requireOpen() | ||
| return withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
|
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. |