New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement `list_root_keys` to the `AdminKeyValueStore` #3142

Open

MathieuDutSik wants to merge 21 commits into linera-io:main from MathieuDutSik:get_root_keys

Contributor

MathieuDutSik commented Jan 16, 2025 •

edited

Loading

Motivation

We need to have the feature of accessing to the list of root keys in order to have mutation function and accessing to the list of chain ids of a storage.

Fixes #3085

Proposal

The implementation causes some problems:

For ScyllaDb the implementation is very easy since the root_key is used as a partition key.
For DynamoDb we can implement the root_key as a partition key, but DynamoDb forbids iterating and determining all the partition keys. So, we need to keep track of the keys.
For RocksDb / StorageService / IndexedDb we need to keep track of the list of root_keys.
For RocksDb, this led to a simplification since the edge case of having a key of the form [255, ..., 255] disappears.
The result of the get_root_keys will not be the same on different storage. If storage has been created with fn create but no key is inserted then in ScyllaDb / DynamoDb it will not show the root_key but in RocksDb it will appear.

The end result is that the fn connect becomes slower for RocksDb, StorageService, IndexedDb but this is I think fine as the issue of performance shows up in other operations.

Test Plan

One test has been added for this feature.

Release Plan

No impact on the TestNet / DevNet. It can follow the normal release plan.

Links

None.

MathieuDutSik marked this pull request as ready for review

January 20, 2025 12:37

afck reviewed

View reviewed changes

linera-views/src/backends/dynamo_db.rs Outdated

@@ @@ -92,11 +92,19 @@ async fn get_config_internal( @@
                   }
               }
+              /// The DynamoDb forbids the iteration over the partition keys.

Contributor

afck Jan 22, 2025

Suggested change

      
            /// The DynamoDb forbids the iteration over the partition keys.
          
            /// DynamoDB forbids the iteration over the partition keys.
          
            ///

Contributor Author

MathieuDutSik Jan 22, 2025

Ok, thanks.

linera-views/src/backends/dynamo_db.rs

+              /// The DynamoDb forbids the iteration over the partition keys.
+              /// Therefore we use a special partition_key named [1] for storing
+              /// the root keys. For normal root_keys, we simply put a 0 in front
+              /// therefore no intersection is possible.

Contributor

afck Jan 22, 2025

Shouldn't this comment be on PARTITION_KEY_ROOT_KEY?
Currently it's associated with PARTITION_ATTRIBUTE.

Contributor Author

MathieuDutSik Jan 22, 2025

Yes, corrected.

linera-views/src/backends/dynamo_db.rs Outdated

-                  let mut vec = vec![0];
-                  vec.extend(root_key);
-                  vec
+                  let mut start_key = vec![0];

Contributor

afck Jan 22, 2025

This is meant to be the same as EMPTY_ROOT_KEY, is it?

Suggested change

      
                let mut start_key = vec![0];
          
                let mut start_key = EMPTY_ROOT_KEY.to_vec();

Contributor Author

MathieuDutSik Jan 22, 2025

Ok.

linera-views/src/backends/dynamo_db.rs Show resolved Hide resolved

linera-views/src/backends/dynamo_db.rs Outdated

@@ @@ -308,7 +314,7 @@ impl TransactionBuilder { @@
                       value: Vec<u8>,
                       store: &DynamoDbStoreInternal,
                   ) -> Result<(), DynamoDbStoreInternalError> {
-                      let transact = store.build_put_transact(&self.root_key, key, value)?;
+                      let transact = store.build_put_transact(&self.start_key, key, value)?;

Contributor

afck Jan 22, 2025

(transact is an uncommon abbreviation for transaction. I'd either use the full word or txn maybe?)

Contributor Author

MathieuDutSik Jan 22, 2025

Ok, I used transaction in the DynamoDb code.

afck reviewed

View reviewed changes

linera-client/src/client_options.rs Outdated

@@ @@ -964,6 +964,14 @@ pub enum DatabaseToolCommand { @@
                       #[arg(long = "storage")]
                       storage_config: String,
                   },
+                  /// List the rot keys of the database

Contributor

afck Jan 22, 2025

Suggested change

      
                /// List the rot keys of the database
          
                /// List the root keys of the database

Contributor Author

MathieuDutSik Jan 22, 2025

Thank you.

linera-client/src/storage.rs Outdated

+                      match self {
+                          StoreConfig::Memory(_, _) => Err(ViewError::StoreError {
+                              backend: "memory".to_string(),
+                              error: "list_all is not supported for the memory storage".to_string(),

Contributor

afck Jan 22, 2025

Suggested change

      
                            error: "list_all is not supported for the memory storage".to_string(),
          
                            error: "list_all_root_keys is not supported for the memory storage".to_string(),

Contributor Author

MathieuDutSik Jan 22, 2025

Thanks.

linera-storage-service/proto/key_value_store.proto Outdated

+              }
+              message ReplyInsertRootKey {
+              }

Contributor

afck Jan 22, 2025

(We could use google.protobuf.Empty instead of all those empty types. Not sure if that's preferable, but we do it in rpc.proto.)

Contributor Author

MathieuDutSik Jan 22, 2025

Thank you, that is great.

linera-views/tests/admin_tests.rs Outdated

+                  root_key_admin_test::<DynamoDbStore>().await;
+              }
+              #[cfg(with_scylladb)]

Contributor

afck Jan 22, 2025

Might be more readable to use test_case for this.

Contributor Author

MathieuDutSik Jan 22, 2025

Yes, thank you.

linera-views/src/store.rs

                   /// Obtains the list of existing namespaces.
                   async fn list_all(config: &Self::Config) -> Result<Vec<String>, Self::Error>;
+                  /// Lists the root keys of the namespace.
+                  /// It is possible that some root_keys have no keys.

Contributor

afck Jan 22, 2025

Do you mean "some namespaces have no root keys"?

Contributor Author

MathieuDutSik Jan 22, 2025 •

edited

Loading

No, I really mean that.
What happens is that for each key, there is at least one root key.
But for some root key, there is no corresponding key.

For ScyllaDb, we will have that the insertion is done via (root_ley, key) which lead to the fact that for each key we have a matching root_key.
For others the root key has to be stored as a key. That creation is done just at fn connect or fn clone_with_root_key. So, the creation itself creates root keys. But the key is not yet written.

What this designs allow is to access to all the keys and so to to transfer files from one storage to another.

afck reviewed

View reviewed changes

Contributor

afck left a comment

Sorry, I think I need some more explanations (or better: code comments, or pointers where to find them) before I can review this. I'm confused how the keys are structured and represented in the different databases. E.g. I thought the first byte was there to distinguish between actual key-value pairs, and markers for the existence of namespaces; in what way does that relate to root keys at all, and why is it changing now?

linera-views/src/test_utils/mod.rs

+                  let set_root_keys = root_keys.iter().cloned().collect::<HashSet<_>>();
+                  for read_root_key in &read_root_keys {
+                      assert!(set_root_keys.contains(read_root_key));
+                  }

Contributor

afck Jan 22, 2025

This only asserts that read_root_keys are a subset of set_root_keys. Shouldn't we assert that they are equal?

Contributor Author

MathieuDutSik Jan 22, 2025

No.
For a database like ScyllaDb, the root keys would be inserted only if there is a corresponding key. This is why there is a size_select that could be 0.

linera-views/src/backends/scylla_db.rs

+                      let mut root_keys = HashSet::new();
+                      while let Some(row) = rows.next().await {
+                          let (root_key,) = row?;
+                          let root_key = root_key[1..].to_vec();

Contributor

afck Jan 22, 2025

Why do we drop the first byte? Is this KeyTag::Key?

Contributor Author

MathieuDutSik Jan 22, 2025

In ScyllaDb, a key has to hava non-zero length. Therefore, we have a function get_big_root_key that adds a 0 at the first entry.

linera-views/src/backends/rocks_db.rs

-                                  }
+                                  let mut full_key1 = self.start_key.to_vec();
+                                  full_key1.extend(&key_prefix);
+                                  let full_key2 = get_upper_bound_option(&full_key1).unwrap();

Contributor

afck Jan 22, 2025

(Why can't this be None? Is this because the edge case is gone, because the first byte is never 255?)

Contributor Author

MathieuDutSik Jan 22, 2025

Yes, exactly.
Added, documentation.

MathieuDutSik changed the title ~~Implement get_root_keys to the AdminKeyValueStore~~ Implement list_root_keys to the AdminKeyValueStore

MathieuDutSik added 21 commits

January 24, 2025 14:09


          Insert the code for the basic stores.

1a6fc76


          Implement the stuff for the ScyllaDb.

db29208


          Implement the feature for DynamoDb.

d4f5590


          Add now the test for RocksDb.

fc4d4ef


          Change the structure of the "fn build" and implement the root_key

e25c8e1

effectively.


          Rename the admin_test function.

affe4b2


          Make clone_with_root_key async.

7b2dc86


          Some insertion of the code.


          Add more tests.

655d379


          Formatting corrections and introduce the get_root_keys to the indexddb.

3f5aed2


          Clippy corrections.

1c93734


          Rename effective_root_key to start_key.

72f1ae5

Implement the feature to the linera-storage-service.


          formatting correction.

4c82b70


          Several more corrections on the whole code.

d40a36f


          Resolve the problem of the rocks_db test.

c689b9e


          Some simplification and correction of full_keys2 entry.

683ed23


          Rename the "get_root_keys" as "list_root_keys".

86742ce

Make the "list_root_keys" available from the CLI.


          Correct the test code + Correct the DynamoDb/ScyllaDb codes.

9c87628


          Some renaming.

fbb0ec7


          Cleanup of the code from the review.

29975ab


          Add the functionality to the documentation.

3e78982

MathieuDutSik force-pushed the get_root_keys branch from 094fd65 to 3e78982 Compare

January 24, 2025 13:09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet