feat: synchronize value for synchronized ranges #238
Conversation
| cat: [a: ∅, b: ∅, c: ∅] | ||
| -> value_req(b) | ||
| cat: [a: ∅, b: ∅, c: ∅] | ||
| <- value_resp(a: A) |
There was a problem hiding this comment.
This will be deterministic? cat will send value requests for a and b, and dog will send a value response for a before cat sends the value request for c?
There was a problem hiding this comment.
Along those lines.. in #243, I'm seeing some ordering changes to req/resp values cause failures and was hoping to get some help understanding what should happen and if it's a problem.
There was a problem hiding this comment.
I regenerated the expect tests for my PR and they seem to be consistent in the that order for now. Will cause some more conflicts for you merging back in.. hopefully it's not bad. It should be merged shortly.
There was a problem hiding this comment.
So its not perfectly deterministic but it does seem to be stable for a given implementation. Meaning I do not expect the tests to be flaky but changes in the performance of the code might change the order.
The test driver explicitly uses a single thread to drive the message exchange, but its still possible because of the async nature of the code that messages can be delivered in a different order.
If we discover that the tests are somehow flaky we can explore ways to improve them. For example we could remove the assertions about state from each message and then allow for messages to be out of order using a new syntax. However I'd prefer to avoid that complexity if possible.
TL;DR we might have to update the expectations when we change to code but the tests should be stable.
| &mut self, | ||
| range: RangeOpen<Self::Key>, | ||
| ) -> Result<Vec<Self::Key>> { | ||
| let query = sqlx::query( |
There was a problem hiding this comment.
when we need to check if the range is valid? In hash_range, we check if left >= right and return 0 but in other range related queries we don't.
There was a problem hiding this comment.
looks like you updated this (or I just missed it before). I'm still curious if this is a concern of storage trait or it's going to be verified higher at the recon client/server level, but nothing to do.
| cat: [a: ∅, b: ∅, c: ∅] | ||
| -> value_req(b) | ||
| cat: [a: ∅, b: ∅, c: ∅] | ||
| <- value_resp(a: A) |
There was a problem hiding this comment.
Along those lines.. in #243, I'm seeing some ordering changes to req/resp values cause failures and was hoping to get some help understanding what should happen and if it's a problem.
nathanielc
force-pushed
the
feature/ws1-1427-events-synchronize-correctness-tests
branch
from
ce2c769 to
3b58c64
Compare
nathanielc
force-pushed
the
feature/ws1-1427-events-synchronize-correctness-tests
branch
from
b4da4db to
83b8f75
Compare
| @@ -91,45 +97,64 @@ where | |||
| // we insert the value first as it's possible we already have the key and can skip that step | |||
| // as it happens in a transaction, we'll roll back the value insert if the key insert fails and try again | |||
| if let Some(val) = item.value { | |||
| if self.insert_value_int(item.key, val, conn).await? { | |||
| // Update the value_retrieved flag, and report if the key already exists. | |||
| let key_exists = self.update_value_retrieved_int(item.key, conn).await?; | |||
There was a problem hiding this comment.
do we want to do this after inserting the value? it seems like we might have an issue if we try to set retrieved, but the key doesn't exist, and then we add the key with retrieved=false.
we could potentially modify insert_value_int to do this update as well and pass a bool to insert_key_int for value_retrieved
There was a problem hiding this comment.
doh, that's exactly what you did lol. nice! probably worth combining the update/insert in a future PR as we optimize some of these queries, but nothing to do now.
There was a problem hiding this comment.
Yeah, I was just going to suggest combining the update/insert then saw your comment on refreshing the page, @dav1do 😄
There was a problem hiding this comment.
Ha! I didn't see yours when I hit resolve! I re-opened it, but I'm okay not addressing it now.
| &mut self, | ||
| range: RangeOpen<Self::Key>, | ||
| ) -> Result<Vec<Self::Key>> { | ||
| let query = sqlx::query( |
There was a problem hiding this comment.
looks like you updated this (or I just missed it before). I'm still curious if this is a concern of storage trait or it's going to be verified higher at the recon client/server level, but nothing to do.
This change updates the Recon protocol to check for any missing values when a synchronized range of keys is discovered. This way as nodes are synchronized they ensure they also have all values for their known keys.
nathanielc
force-pushed
the
feature/ws1-1427-events-synchronize-correctness-tests
branch
from
067ea62 to
2ada06c
Compare
nathanielc
deleted the
feature/ws1-1427-events-synchronize-correctness-tests
branch
This change updates the Recon protocol to check for any missing values when a synchronized range of keys is discovered. This way as nodes are synchronized they ensure they also have all values for their known keys.
TODO