[DEMO: don't merge] Trial kvstore interface cleanup #836
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
I think this is a good change. Makes even more sense when we merge #835
All database methods are accepting context, but I don't think they fully support it as they do not check the cancellation status. Usually the database driver should do this work. In our case the database is not context aware.
Should we add at the beginning of each or the core methods check if the context was cancelled?
switch {
case <-ctx.Done():
return nil, ctx.Err()
default:
// Context not cancelled, proceed.
}
This makes sense. I think there is no cancel in iavl, except for the long-running processes, which we do handle with a context. That said, I like your idea of manually checking cancel in the beginning, so it does have some effect (even if it doesn't abort in middle of the I/O, it will break out long-running processes, like with a timeout) |
|
Change seems good to me, and it would indeed be cool to have methods exit on context cancellation, effectively making them a noop if they were just called. |
|
I would love to merge this. There are a lot of conflicts by now. I can resolve all conflicts and bring the code to mergable state. |
|
It does not seem like a good idea to do merge this now as we have decided to hold off on any interface breaking changes that touch multiple packages. We either have to give up on the idea of keeping master launch-compatible, which in my opinion is an okay approach, or keep this open till we have a way forward. To make this decision we'll need to have some direction from business. |
ethanfrey
changed the title
|
I think this should be done as a post-launch overhaul. It changes code, but doesn't break any external guarantees (how blockchain txs execute). So easy enough to add in an iov chain upgrade, say 1.0 -> 1.1 |
Closes #823
Modify the interfaces as discussed in issue. And update
./storeto use the new interface.All other code, like Buckets, is not updated yet. Waiting for feedback if to continue with this or change course