SubscribeMulti + UnsubscribeMulti, multiplicities #249
Conversation
|
I changed the base branch to |
|
Out of curiosity, why did the DLLs need updating? |
kazimuth
changed the title
|
Some missing or dangling .meta found. Fix commits are needed.
|
1 similar comment
|
Some missing or dangling .meta found. Fix commits are needed.
|
|
Some missing or dangling .meta found. Fix commits are needed.
|
3 similar comments
|
Some missing or dangling .meta found. Fix commits are needed.
|
|
Some missing or dangling .meta found. Fix commits are needed.
|
|
Some missing or dangling .meta found. Fix commits are needed.
|
|
Blackholio tested on clockworklabs/Blackholio#22 |
|
@bfops uhh what branch should this be based on? |
|
@kazimuth if I'm reading this correctly, inserts are applied to the cache before deletes, correct? |
|
Good catch -- but that loop is accumulating into a MultiDictionaryDelta, which is a collection of key-value pairs with signed multiplicities. Since the multiplicities can go negative, you're allowed to remove a row before you insert it; in general, the order Adds and Removes are applied to a MultiDictionaryDelta doesn't matter. The resulting delta will have the same effect when Applied to a MultiDictionary. The proptest ShuffleDelta tests this. I should definitely comment about this, thank you. |
|
@bfops , possibly the DLLs did NOT need updating and I did it unnecessarily. I just ran my update-everything script out of habit, but I can undo that. Update: Done. |
|
@rekhoff I'm wondering if we should put that multiplicity test into its own PR. I think we probably want to merge this and add that test later. It will require some git-fu I can help out with. |
kazimuth
force-pushed
the
jgilles/unsubscribe-fix
branch
from
fce7dc6 to
333efc0
Compare
Warning message fix (thanks Jeff) Update for new Subscribe/UnsubscribeMulti thanks zeke Add MultiDictionary; update dispatching logic still wrong Add even more wacky data structures WHOOOOOO Fix MultiDictionary bug More asserts and comments Tests passing Better errors, purge dead package Test Update for blackholio Update blackholio Fix comment Remove dead One more dead COMMENT More comments Added a multiplicity test example Creates a small sample program with a Rust server and C# client that tests various subscriptions and outputs the results to the CLI. Make sure we insert before we delete Fix more comments Add CRDT test for MultiDictionaryDelta Revert "Added a multiplicity test example" This reverts commit ff10925. Undo DLL changes
kazimuth
force-pushed
the
jgilles/unsubscribe-fix
branch
from
333efc0 to
2fde31b
Compare
|
@rekhoff, I moved your code to the branch |
kazimuth
force-pushed
the
jgilles/unsubscribe-fix
branch
from
3854092 to
2fde31b
Compare
|
John: What's the story on the subscription helper? |
|
Now based on clockworklabs/SpacetimeDB#2303 |
There was a problem hiding this comment.
I've reviewed the MultiDictionary and Table cache changes for tracking multiplicities and invoking row callbacks. Just a small logging nit, but otherwise the logic is correct.
As for the rest of the changes, I'll defer to @rekhoff.
Description of Changes
Add SubscribeMulti and UnsubscribeMulti from upstream.
Fix unsubscribe bug found by testing against Bitcraft: Calling
UnsubscribeThenwith any (non-null) callback would result in an exception incorrectly telling the user thatUnsubscribehad been called twice.Multiplicity support is implemented with unit tests + manual testing of quickstart-chat.
Also, rows in the client cache are now looked up by primary key if available, which I suspect is going to be a large performance boost.
API
If the API is breaking, please state below what will break
Requires SpacetimeDB PRs
List any PRs here that are required for this SDK change to work
Testsuite
If you would like to run the your SDK changes in this PR against a specific SpacetimeDB branch, specify that here. This can be a branch name or a link to a PR.
SpacetimeDB branch name: jgilles/final-cs-codegen-changes
Testing
Write instructions for a test that you performed for this PR