Skip to content

Iteration over storage items with given key prefix - #411

Merged
iljakuklic merged 3 commits into
masterfrom
storage-iter
Sep 14, 2022
Merged

iljakuklic merged 3 commits into
masterfrom
storage-iter

Conversation

@iljakuklic

@iljakuklic iljakuklic commented Sep 9, 2022 •

Copy link
Copy Markdown
Contributor
  • Backend and frontend interfaces
  • In-memory implementation
  • Tests
  • Some refactoring, mostly just moving stuff into submodules and similar

Comment thread storage/backend-test-suite/src/basic.rs Outdated
Comment thread storage/core/src/adaptor/locking/mod.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct way to do this is by having an Operation enum, just like in TransactionVerifier. The bonus you get there is that you can support caching read-operations by adding an extra arm, which you can exclude from writing back to db when you commit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can have a look into it but preferably not as a part of the present PR. There will have to be some thought around how to mutate the data structure.The get() method now takes &self, not &mut self so some decision whether to introduce mutability into the interface or use some interior mutability mechanism would have to be made.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. I don't mind doing this in another PR. But I also want to point out that even without reads, mapping Some to Write and None to Erase is confusing. Let's fix this some times later with or without reads.

Comment thread storage/core/src/adaptor/locking/prefix_iter_rw.rs Outdated
Comment thread storage/backend-test-suite/src/basic.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test of iteration that starts with a set, then deletes some items of the set arbitrarily in the middle, and then iterates again and ensures only the correct items are left?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I meant within and outside of a transaction.

This can be seen in multiple cases:

  1. Create the data, commit, erase a subset, check, commit, check
  2. Create the data, erase a subset, check, commit, check

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I meant within and outside of a transaction.

This can be seen in multiple cases:

  1. Create the data, commit, erase a subset, check, commit, check
  2. Create the data, erase a subset, check, commit, check

These are covered by randomised property tests but will add some normal tests as well since these tend to be easier to analyse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more tests for deleting stuff

Comment thread storage/core/src/util.rs Outdated
Comment thread storage/inmemory/src/lib.rs Outdated
Comment thread storage/src/test.rs Outdated

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I made some minor comments.

This adds an interface for iterating over database entries where keys
start with given prefix. The interface is then implemented for the
in-memory backend.
@iljakuklic
iljakuklic force-pushed the storage-iter branch 2 times, most recently from 5572f2e to a77f743 Compare September 14, 2022 09:01
Mostly tests to do with the recently introduced key prefix iteration but
includes some other new tests and test framework enhancements too.
* Add application-agnostic frontend iteration functionality
* Add a simple test for it
* Move modules around a bit
@iljakuklic
iljakuklic merged commit 0553bd7 into master Sep 14, 2022
@iljakuklic
iljakuklic deleted the storage-iter branch September 14, 2022 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants