Make proposals owned by API caller #161
Conversation
Doing this simplifies the implementation significantly, and only adds some complexity at commit time. Also use Arc<Proposal> everywhere so they can be cloned and referenced by other proposals easily.
| @@ -111,9 +115,9 @@ pub trait Db { | |||
| /// [BatchOp::Delete] operations to apply | |||
| /// | |||
| async fn propose<K: KeyType, V: ValueType>( | |||
| &mut self, | |||
| self: Arc<Self>, | |||
| /// Commit this revision | ||
| /// | ||
| /// # Return value | ||
| /// | ||
| /// * A weak reference to a new historical view | ||
| async fn commit(self) -> Result<Weak<T>, Error>; | ||
| async fn commit(self) -> Result<T, Error>; |
There was a problem hiding this comment.
What happens when two threads try to commit two conflicting proposals at the same time?
Scenario:
P1 = R1 + [Put(K1, V1)]
P2 = R1 + [Put(K2, V2)]
t0 -> P1.commit()
t0 -> P2.commit()
Just to be clear, these are two competing but not conflicting proposals.
There was a problem hiding this comment.
You actually can't create a proposal from two different revisions, unless a commit happens between them, so I'm not sure how you are creating P1 and P2.
There was a problem hiding this comment.
edited, should have said R1 in both cases
There was a problem hiding this comment.
In this case, when P2 commits, it will see the base revision is not R1 (it is now R2) and will return an invalid proposal.
| @@ -39,40 +38,27 @@ impl api::Db for Db { | |||
| } | |||
|
|
|||
| async fn propose<K: KeyType, V: ValueType>( | |||
There was a problem hiding this comment.
Why propose on the database at all, why not limit proposal creating to be based on revisions?
There was a problem hiding this comment.
Ah, because then you get the scenario above! You cannot propose anything except against the most recent version of the database or another proposal (which is based on the most recent version of the database). If you did, the proposal would immediately be invalid, so what would be the point?
There was a problem hiding this comment.
I've talked about this on calls and on slack, just wanted to get it down in GitHub somewhere as well. We should be consistent. We either put all the onus on the user to ensure they're working with valid data OR we only allow them to create valid proposals.
Currently, we enforce valid proposal creation on the database, but don't enforce anything when creating proposals on top of proposals.
I think we could actually do both, allowing the more flexible creation of proposals on top of revisions or other proposals without validating the data through trait implementations behind a commit-only-validation feature flag.
With the feature flag off, I suggest the default behaviour is a Db::propose_on(parent: Proposal, batch: Batch) -> Proposal that validates that the parent proposal is valid before creating a new proposal.
|
|
||
| Ok(Arc::downgrade(&proposal)) | ||
| Ok(Arc::new(proposal)) |
Doing this simplifies the implementation significantly, and only adds some complexity at commit time. Also use Arc everywhere so they can be cloned and referenced by other proposals easily.