feat: Implement Postgres interfaces for KV and Logging #65
Conversation
| tokio::spawn(async move { | ||
| if let Err(e) = connection.await { | ||
| eprintln!("connection error: {}", e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Why is this here? Swallowing an error doesn't seem useful
There was a problem hiding this comment.
Makes sense, do you think it would be better to panic if the connection is broken?
There was a problem hiding this comment.
Maybe just tracing::error! instead? Didn't notice that there's no way for the logging API to surface errors
There was a problem hiding this comment.
I was thinking and the whole approach is kind of off. I'll refactor for the following:
- use a connection pool to handle the connections in the bg, also to make it more resilient to heavy usage (particularly this one)
- given that I would be adding dependencies, make it a optional feature of the runtime crate
I was trying to keep it minimal but I think it makes sense to be robust and optional instead of slim and brittle. Do you agree?
There was a problem hiding this comment.
I think that's a valid approach, but the right thing to do here probably depends on how @scarmuega wants balius-runtime consumers to work.
If the runtime is supposed to be "batteries included", then it makes sense to have a robust (and optional) postgres provider inside of it. If you really want it to be robust, I'd also recommend
- Let the caller reuse connection pools. If the logger and KV store are both using postgres, they should probably be allowed to use the same set of postgres connections.
- Provide some optional "init_db" method that the consumer can call to set up tables. Different software stacks manage DB schemas in different ways, and one of those ways is "the application sets up its own schema at startup/deploy time". Supporting that case is easy.
- Let the caller choose the table name (or maybe make table name a function of the worker). The logs table will be really write-intensive, and the KV table could have lots of reads and writes depending on what the application is doing. You'll likely want sharding to run this in prod (otherwise, bad actors can spam KV writes/deletes and bloat the indexes for everyone), and "different workers use different tables" can be simpler to set up than DB-level sharding.
If it's more important for the runtime to be minimal, then you could make callers implement their own logging/KV solutions themselves. That's supported today; firefly-cardano is using a custom KV store which gives each worker its own table in a sqlite DB, and creates those tables as needed. You could provide implementations of these components in separate crates, and so could third-party consumers. There's not much advantage to that split today, but it makes more sense in a world where balius is multi-chain and you have devs providing their own signer implementations for their chain's cryptographic primitives.
There was a problem hiding this comment.
For what its worth, I'm friendlier with the idea of having this custom implementations outside of Balius. If that's what we want to do, then I would just delete this PR and create a new one that just contiains the KvProvider logic that allows to create scoped kv and logging interfaces per worker. Then have the pg implementations on ext-balius
There was a problem hiding this comment.
@SupernaviX after speaking with santi, I will close this PR with the idea of having the pg interfaces on the demeter extension repo
No description provided.