support new data structure

[L-jasmine]

• Optimize the code structure for new data structure.
• Implement a "causal consistency" HashMap
• Implement a "causal consistency" counter

[phil-opp]

Thanks a lot for this PR! Since it is quite large I started reviewing by commit, currently up to commit d4834dc.

• Commits 9669689 to 0e65f7f look good to me.
• I'm not sure about commit a8e2945 ("feat: add KeyOperation::SetAdd").
  • Why do we need a new KeyOperation for this? Couldn't we just create a new put_causal_set method in src/nodes/client/mod.rs that handles the vector clock on the client-side and then call self.put(key, LatticeValue::SingleCausal(...))?
  • The SingleKeyCausalLattice already implements a merge logic:

    anna-rs/api/src/lattice/causal/single_key_causal.rs

    Lines 42 to 54 in e60629b

    	let prev = self.element.vector_clock.clone();
    	self.element.vector_clock.merge(&other.vector_clock);
    	if self.element.vector_clock == other.vector_clock {
    	// the value in `other` is strictly newer, i.e. in causally follows `self`
    	self.element.value = other.value.clone();
    	} else if self.element.vector_clock == prev {
    	// our value is strictly newer, i.e. `self` causally follows `other -> nothing to do
    	} else {
    	// the value was written concurrently -> merge the values using their `Lattice` impl
    	self.element.value.merge(&other.value);
    	}
    	}

    
    I don't think that it's a good idea to implement a different merge logic in the KVS request handler depending on the request type. In my opinion, there should be one canoncial merge implementation for each type as part of the Lattice trait implementation without any special casing.
  • With this change it would be possible to write the same key using different operations, SetAdd and Put, with different results. This could easily lead to confusion and bugs in practice.
• I have the same concerns about 5674795. Why not set the vector clock at the client side instead and use a normal PUT operation?
• 75f4361, 3763a48, and d4834dc look good to me.

[L-jasmine]

Hi Phillip,

Happy new year! Thank you for the quick review. Let me take a step back and give a little more context on the PR.

Our goal

Our end goal here is to provide a Redis compatible API for storing / managing data structures over anna-rs. It alleviates a key problem Redis faces — it is single threaded and cannot scale well for the cloud.

Overall approach

We initially thought that we can use client side transactions to compose multiple PUT / SET operations for each data structure type. But we concluded that a client-only solution is not possible. Specifically,

• The client-side operations would require too much network chatter, especially when the user needs to modify data objects already stored on the server.
• The client could put together operations that are not compliant with the data structure requirement and the server would require a lot of overhead to detect such operations.

Hence, the approach in this PR is to modify the anna-rs server injunction with the client to support key data structures such as Set and Map. We believe that more complex data structures can be supported more easily later once we have Set and Map.

Why I modified KeyOperation::PUT?

First, the KeyOperation::Put incurs a lot of network overhead when a hashset has a lot of entries. So I added KeyOperation::SetAdd.

Second, It's important to make it impossible for the client to provide illegal input, so I put the vector clock handling on the server side. And in this way, the client doesn't have to care what the vector clock is. So I also modified KeyOperation::PUT to modify only LwwLattice.

About the merge logic

The Lattice::merge can ensure that the outputs are finally consistent if the data on the same key between multiple kvs is detected to be in conflict.

What I did in the request handle was not lattice merge. I just modified the key data in kvs as requested by request.

In multiple kvs Gossip handlers, the lattice in the same key will resolve conflicts through the merge method. This is the only place where I used merge.

The last question

As I mentioned, the Gossip handler will handle the conflicts, so we will finally get the subsequent output. It is allowed by Anna's designs that we have some conflicts during the process.

I hope these explanations clarified the intention and approach behind these changes. If you see better ways to achieve Redis API support, please do let me know. Thanks!

[phil-opp]

Thanks a lot for clarifying and happy new year to you too!

I understand your points about the network overhead of modify operations and the need for server-side validation. However, I'm still not sure if it's a good idea to extend the KeyOperation enum directly. Would it be possible to possible to introduce a new ClientKeyOperation enum with all the client-specific operations and then translate them to fundamental Get or Put operations? For example, a ClientKeyOperation::SetAdd could read the current value from the kvs, add the new values, and then create a Put operation with the modified value. This way, the KeyOperation type would stay general and extensible, e.g. in case we want to implement a different client API in the future.

[L-jasmine]

Thanks! Let me try and create a ClientKeyOperation as you suggested. Will update soon.

[phil-opp]

I just saw that you pushed some new commits. Is this ready for another round of review?

[juntao]

Hi,

We are using a GitHub bot to automatically generate code review summaries from ChatGPT/4 for PRs in this repo. Please excuse the construction as we are optimizing the prompts.

Here I am going to the magic words to summon the bot: flows summarize

[phil-opp]

@juntao Thanks for the summary!

@L-jasmine Is this PR ready for another round of review from your side?

[L-jasmine]

Oh, I'm sorry for replying so late. Yes, I'm ready for another round of review now. I noticed that there are some conflicts, but I will resolve them later. Please proceed with the review for now. Thank you.

[juntao]

We upgraded the bot to GPT4. So, I will summon it to summarize again. :) Feel free to delete the summaries if they clutter up the timeline. Thanks!

flows summarize

[juntao]

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.

---

Overall, this set of patches introduces various changes to the source code, including adding new functions, refactoring existing code, modifying data structures, and changing serialization and deserialization libraries.

Potential issues and errors:

1. Assumptions about 'Client' variant in 'try_request' method, no check to ensure this, leading to unexpected behavior.
2. Inefficient Vec creation in 'keys' method in RequestData, leading to performance issues.
3. Compatibility, performance and error handling issues due to replacing 'serde_json' with 'rmp-serde' library.
4. Possible concurrency issues and lack of updates for other lattice types with the new 'reveal_mut' method.
5. Potential issues with the new 'issue_replication_factor_request' function and its impact on the overall structure and flow of the program.
6. Moving 'ResponseType' field from 'Response' to 'ResponseTuple' struct might cause issues with code accessing the old field location.

Important findings:

1. The refactoring results in simpler and easier-to read code, improving maintainability.
2. Introducing the new 'hash_ring_util.rs' file helps with better code organization.
3. Changing the log level might make logs more verbose, which could be problematic if logs grow too large.
4. Modified data structures such as 'KeyOperation', 'RequestData' and 'ResponseTuple' might cause issues if not handled correctly.
5. Added new functions such as 'try_get_responsible_threads' and 'redirect_gossip' improve the modularity of the codebase.
6. Splitting 'KeyOperation' into different types for client keys and metadata keys adds clarity to the code.
7. The removal of the 'lattice' submodule from the client might impact functionality and lead to compatibility issues.

Due to these changes, it is essential to thoroughly test the updated code, ensure proper error handling, and verify compatibility with older clients and external dependencies. Moreover, the refactoring should be monitored for any impacts on performance and maintainability.

Details

Commit 1

Summary of key changes:

1. Added a new function keys() in RequestData to return keys for a request.
2. Modified the ClientRequest struct to use a new KeyOperation enum instead of separate fields for key and put_value.
3. Refactored the conversion from ClientRequest to Request using a match expression for KeyOperation.
4. Updated various parts of the ClientNode implementation to work with the changed ClientRequest structure.

Potential problems:

1. The implementation assumes that the Key will always be of the 'Client' variant while handling the 'try_request' method. There seems to be no check to ensure this, and other variants might cause unexpected behavior.
2. The keys method in RequestData creates a new Vec for each call. This could be inefficient if called frequently. Perhaps returning a reference would be a better approach.

Commit 2

Summary of key changes:

1. Replaced serde_json with rmp-serde for serialization and deserialization.
2. Removed serde_json in Cargo.toml and added rmp-serde.
3. Updated serde_json related function calls to use corresponding rmp-serde functions.
4. Changed some log levels and the number of benchmark threads in the example-config.yml.

Potential problems:

1. Compatibility Issue: If the new format is not backward-compatible with older clients or older versions expecting serialized data in JSON format, this could cause issues.
2. Performance Impact: Although the change might have been done to improve performance, it's always better to perform benchmark tests to confirm the expected performance improvement.
3. Error Handling: Error handling might need to be updated for the new serialization library, as the error messages might be different.

Commit 3

This patch introduces the following key changes:

1. A new reveal_mut method is added to the Lattice trait:

fn reveal_mut(&mut self) -> &mut Self::Element;

This method returns a mutable reference to the current value stored in the lattice.

2. The reveal_mut method is implemented for 9 different lattice types, including BoolLattice, MultiKeyCausalPayload, VectorClockValuePair, TimestampValuePair, HashMap, MaxLattice, BTreeSet, and HashSet.

Potential problems:

1. Since the patch introduces mutable access to the lattice elements, it may inadvertently introduce unsynchronized modifications or other concurrency issues if reveal_mut is used improperly. It is important to ensure that the API documentation highlights this risk and provides necessary guidance on safe usage.

2. This patch only covers 9 lattice types, so if there are other lattice types in the codebase, they may require similar updates for consistency. Make sure all relevant lattice types have been updated to include the new reveal_mut method implementation.

Commit 4

Summary:
The patch mainly refactors the code by simplifying the logic in multiple handlers and moving the repeated code to a separate function. It also introduces a new file named hash_ring_util.rs that contains a new method called issue_replication_factor_request. Also, the log level has been changed to Info. The key changes can be summarized as follows:

1. Created new hash_ring_util.rs file with a new issue_replication_factor_request function.
2. Refactored kvs.rs by changing the log level from Warn to Info.
3. Refactored gossip.rs, replication_response.rs, and request.rs handlers by simplifying them and removing repeated code.
4. Refactored ConfigData and PendingRequest structures.

Potential issues:

1. The new function issue_replication_factor_request is changing the overall structure and flow of the program. There could be potential issues with this function. The programmer should thoroughly test the function to ensure it works as expected.
2. Changing the logging level from Warn to Info might make the logs more verbose, which could be problematic if logs grow too large.

Important findings:

1. Refactoring the code and removing repeated logic results in simpler and easier-to-maintain code.
2. Introducing the new hash_ring_util.rs file is a positive step as it helps in better code organization.
3. Changing the log level might result in larger logs; developers should be cautious about this change.

Commit 5

Summary of key changes:

1. Move the ResponseType field from the Response struct to the ResponseTuple struct.
2. Remove the ResponseType field from the generate_bad_response(), replication_response_handler(), and new_response() functions in various files.
3. Add the ResponseType field to the ResponseTuple creation in various files.

Potential problems:

1. Since ResponseType is moved from the Response struct to the ResponseTuple struct, there may be other code areas that still access the old field location and cause issues.
2. Functions and tests that create new Response or ResponseTuple objects might need to be updated for the new structure, as demonstrated in the files changed.

It's essential to verify all code paths referencing ResponseType and ensure they have the correct behavior based on the new data structure. Additionally, thorough testing should be conducted to ensure these changes don't introduce bugs or impact system performance.

Commit 6

This patch renames PutTuple to ModifyTuple in the codebase. The key changes can be summarized as follows:

1. In src/messages/request.rs, the PutTuple struct is renamed to ModifyTuple.
2. All occurrences of PutTuple are replaced with ModifyTuple in the following files:

• src/nodes/client/mod.rs
• src/nodes/kvs/gossip.rs
• src/nodes/kvs/handlers/gossip.rs
• src/nodes/kvs/handlers/replication_response.rs
• src/nodes/kvs/handlers/request.rs
• src/nodes/kvs/report.rs
• src/nodes/monitoring/mod.rs

Potential problems:

1. The patch only changes the name PutTuple to ModifyTuple, and there don't seem to be any code logic changes or potential problems introduced by the renaming itself.
2. If other unpatched code or external dependencies still refer to PutTuple, they might break due to this change. It is essential to ensure that all dependencies or related codebases are updated with the new name to prevent any issues.

Overall, assuming that all references to the renamed struct have been updated accordingly, this patch should not introduce any significant problems.

Commit 7

Summary of key changes:

1. A new variant Gossip is introduced in the RequestData enum.
2. The code for handling gossip requests is updated to use Gossip variant instead of Put.
3. The redirect_gossip function is introduced to handle the gossip redirection in a more modular manner.
4. Test cases are updated to use the newly introduced Gossip variant.

Potential problems:

1. Using log::warn on receiving a gossip request in the RequestData keys function might not be optimal, as it could lead to unnecessary log spam.

Commit 8

Summary of key changes:

1. The Operation::Get and Operation::Put requests have been merged into a single Operation variant in RequestData enum. This new variant contains a vector of KeyOperation objects.
2. The occurrences of Get and Put requests in various parts of the code have been replaced with their corresponding KeyOperation::Get and KeyOperation::Put variants.

Potential problems:

1. Incorrect handling of request types: Since Get and Put requests have been merged into a single Operation variant, there is a risk that some parts of the code may not handle this change correctly, leading to unexpected behavior.
2. Increased complexity and reduced readability: Merging the Get and Put requests into a single variant might make the code more complex and harder to read, especially for new developers or those less familiar with the codebase.
3. Possible performance impact: If the handling of the new Operation variant is not properly optimized, there might be a performance impact due to the additional vector of KeyOperation objects.

Commit 9

Summary of Key Changes:

1. Added a new KeyOperation::SetAdd variant to handle merging a single-key causal lattice into the key.
2. Added a create_with_default_clock constructor method for SingleKeyCausalLattice.
3. Updated the client node and interactive client node to include the new add_set method for updating the key-value store.
4. Modified the KVS node handler to deal with the KeyOperation::SetAdd operation.

Potential Problems:

1. There is a possibility that the new KeyOperation::SetAdd variant might affect other key operation handling if not handled correctly.
2. The newly added create_with_default_clock must be used cautiously as creating the default clock might affect the causal relationship in the lattice operations.
3. The modification in the KVS node handler might cause issues if the matched entries are not of the expected LatticeValue type.

Commit 10

Summary of key changes:

1. Added a new Causal HashMap type as a LatticeValue variant called SingleCausalMap.
2. Modified KeyOperation enum with a new variant called MapAdd.
3. Added a new ResponseType enum variant called MapAdd.
4. Implemented an ADD_HASHMAP command in the client interactive mode.
5. Added the add_map_async method in the client module to initiate a MapAdd request.
6. Added the add_map method in the client module to make the request and wait for the response.
7. Handled the new KeyOperation::MapAdd variant in the KVsNode handler to manage hashmap addition to KVS.

Potential problems:

1. Code duplication due to a similar structure in the ADD_SET functionality. This may lead to potential maintenance issues.
2. Error handling or input validation might be needed for the ADD_HASHMAP command in the client's interactive mode.

Commit 11

This patch adds a new client command called display primarily for debugging purposes. The changes are made in three files:

1. src/nodes/client/display.rs: A new file is created defining the LatticeDisplay trait with its various implementations for different structures like LastWriterWinsLattice, SetLattice, MapLattice, MaxLattice, and SingleKeyCausalLattice. The trait introduces a method display() to display the contents of the specific data structure, with appropriate context if required.

2. src/nodes/client/interactive.rs: A new case for the display command is added in the match block. It takes a key as an argument, retrieves the value from the key-value store, and displays it using the display() method.

3. src/nodes/client/mod.rs: A new reference to the display module is added.

Potential issues:

• There's no mention or example of how to use the new display command in the help message, making it difficult for users to discover or understand its purpose.
• Error handling for key-value retrieval is minimal; mainly ensuring that the value received is valid UTF-8. More sophisticated handling may be beneficial.

Commit 12

This patch updates a test file named basic.rs. The key changes include updating PUT_SET to ADD_SET in two lines of the test. This change may affect the behavior of the test, so it should be reviewed thoroughly.

Summary of Changes:

1. Replaced PUT_SET set 1 2 3 with ADD_SET set 1 2 3
2. Replaced PUT_SET set 1 2 4 with ADD_SET set 1 2 4

Potential problems:

1. If ADD_SET has different behavior than PUT_SET, it might cause the test to fail or behave unexpectedly.
2. No changes have been made to the implementation code. If PUT_SET has been replaced with ADD_SET in the implementation code as well, it should also have been included in this patch.
3. There might be other occurrences of PUT_SET in the codebase that should be updated to ADD_SET. Be sure to check if this change is consistent throughout the codebase.

Commit 13

Summary of key changes:

1. RequestData enum has been replaced with a struct, and it now contains only one field operations: Vec<KeyOperation>.

2. A new file src/messages/gossip.rs has been created. It contains gossip-related structures:

   • GossipDataTuple, a struct representing a key-value pair.
   • GossipRequest, a struct containing the list of gossip updates as tuples: Vec<GossipDataTuple>.

3. Removed the into_tuples() method from RequestData.

4. Modified handlers and other functions to adhere to the new structure of RequestData and GossipRequest.

Potential problems and suggestions:

1. Functions like generate_bad_response() and redirect_gossip() can be optimized after these refactoring changes.

2. Ensure the rest of the codebase has been properly refactored and updated to match these changes.

Commit 14

The patch mainly introduces a new function try_get_responsible_threads to the KvsNode struct and refactors existing code to use this new function.

Key changes:

1. Add implementation of try_get_responsible_threads to src/nodes/kvs/hash_ring_util.rs. This function is now part of KvsNode and wraps the previous hash_ring_util.try_get_responsible_threads call, simplifying the call signature and defaulting the tiers parameter to the self.config_data.self_tier when it isn't provided.

2. Refactor the code in the following files to use the new try_get_responsible_threads function:

   • src/nodes/kvs/gossip.rs
   • src/nodes/kvs/handlers/gossip.rs
   • src/nodes/kvs/handlers/node_join.rs
   • src/nodes/kvs/handlers/replication_response.rs
   • src/nodes/kvs/handlers/request.rs
   • src/nodes/kvs/handlers/self_depart.rs

3. Simplify the drain usage in src/nodes/kvs/gossip.rs and src/nodes/kvs/handlers/node_join.rs by replacing the for loop with a collect() call on the iterator.

Potential problems:

1. The removal of error context in src/nodes/kvs/handlers/node_join.rs might make it harder to understand the source of an error if it occurs. Consider keeping the context to aid debugging.

Commit 15

Summary of key changes:

1. Split the KeyOperation enum into different types for client keys and metadata keys.
2. Removed the ModifyTuple struct and replaced its usage with direct tuple parameters inside the new KeyOperation variants.
3. Updated usages of KeyOperation and removed the mention of RequestData since it is no longer required.
4. Created a new file src/nodes/kvs/store.rs.

Potential problems:

1. There could be issues with backward compatibility, since the previous representation of the Request and KeyOperation has been changed.
2. Need to ensure that all the calls and pattern matches for the new KeyOperation variants are correctly handled.

Commit 16

Summary of Changes:

1. In src/messages/request.rs, the value type of PutMetadata enum variant is changed from LatticeValue to LastWriterWinsLattice<Vec<u8>>.
2. In src/nodes/kvs/report.rs, the PutMetadata usage is updated to remove the LatticeValue::Lww wrapping and directly use LastWriterWinsLattice::from_pair.
3. In src/nodes/kvs/store.rs, the self.kvs.put method call for KeyOperation::PutMetadata is updated to wrap the value with LatticeValue::Lww.
4. In src/nodes/monitoring/mod.rs, the PutMetadata usage is updated to remove the LatticeValue::Lww wrapping and directly use LastWriterWinsLattice::from_pair.

Potential problems:

1. By changing the value type of PutMetadata, it may break the code that relies on the old type, specifically where the old type is explicitly mentioned or pattern matched. However, the changes made in this patch update related code usages to ensure compatibility.
2. As the changes made directly affect how metadata is handled, it is essential to verify that this new implementation does not impact the performance or correctness of the entire system.

Commit 17

The key changes in this patch include:

1. Splitting response type from LatticeValue.
2. Removing the src/nodes/client/display.rs file.
3. Introducing a new enum ClientResponseValue for different response types.
4. Implementing From<LatticeValue> for ClientResponseValue.
5. Updating functions such as get_lww, get_set, and get_map in src/nodes/client/mod.rs to work with the new ClientResponseValue.
6. Updating functions replication_response.rs and cache_ip.rs to use .metadata instead of .lattice.
7. Several other minor changes and code clean-ups.

Some potential problems in this patch are:

1. The deletion of src/nodes/client/display.rs might impact functionality related to displaying lattice values.
2. The change from using .lattice to .metadata might result in unexpected behavior if it is not handled correctly for all related functions.
3. Possible issues with code compilation and runtime errors given the changes in data structures and associated functions.

Commit 18

Summary of Key Changes:

• The lattice module has been moved from anna_api to anna.
• All imports and module paths have been updated accordingly.
• The patch includes renaming of files and directories.

Possible Problems:

1. The patch contains a lot of file renames, which could lead to merge conflicts if other developers are working on those files.
2. No new functionality or tests are provided, so it's hard to assess if there are any potential issues introduced due to refactoring.

Note: At first glance, the changes seem reasonable given the goal of moving the lattice module from anna_api to anna. However, it's important to ensure that this change does not break any existing functionality or introduce any problems in the application.

Commit 19

This patch makes the following key changes:

1. Hides the lattice submodule from the client, which can be seen in changes to src/bin/benchmark.rs and src/nodes/client/mod.rs.
2. Modifies the KeyOperation enum to remove LatticeValue in favor of simpler Vec<u8> for Put and PutMetadata variants, and updates HashMap for MapAdd and HashSet for SetAdd variants.
3. Updates the ClientNode methods put_async and add_set_async to work with the new KeyOperation changes.
4. Removes the methods put_causal and get_causal from src/nodes/client/interactive.rs.
5. Adds a new timestamp field to src/hash_ring/mod.rs, src/messages/request.rs, and src/nodes/client/client_request.rs.

Potential problems and concerns:

1. The removal of the put_causal and get_causal methods might lead to compatibility issues if there are external dependencies relying on these methods.
2. Changing the KeyOperation enum to use Vec<u8> and removing the LatticeValue might cause altered behavior in the existing code if not handled properly.

Commit 20

Summary of Changes:

• New structures were made public in anna_api
• addr_request.rs, addr_response.rs, messages/mod.rs, request.rs, response.rs, and topics.rs were added to api/src directory
• Minor changes in src/bin/benchmark.rs, src/nodes/client/client_request.rs, and src/nodes/client/interactive.rs
• Key changes in src/nodes/client/mod.rs

Potentials problems:

1. Potential performance implications due to increased number of files and insertion of lines.
2. Checking for any compatibility issues introduced with these newly added modules and structures.
3. Verify proper error handling and serialization/deserialization in new messages.
4. Ensure no overlaps and conflicts in the newly created Zenoh topic paths.

Commit 21

This patch implements a causal counter with the following key changes:

1. Adds the CounterLattice structure to src/lattice/mod.rs, src/lattice/counter.rs. This lattice represents a counter whose increments are causally ordered.

2. Adds the enum variant LatticeValue::Counter(CounterLattice) to src/store.rs, allowing the counter lattice to be used as part of the Key-Value Store operations.

3. Implements the inc method for incrementing the counter with thread id, clock, and value.

4. Adds support for the INC command in src/nodes/client/mod.rs, src/nodes/client/interactive.rs. This client-side command sends a request to increase the value of a key in the KVS when its value is of CounterLattice type.

5. Implements handling for the KeyOperation::Inc variant in src/nodes/kvs/store.rs, updating the corresponding counter value in the Key-Value Store.

6. Modifies the appropriate enum match arms in the code to include the new Inc operation.

One potential issue is that decrementing the counter using a negative increment value will be allowed by this implementation.

Commit 22

This patch includes changes affecting 27 files. The main change is replacing rmp_serde::to_vec with rmp_serde::to_vec_named across the codebase. This suggests a possible refactoring or upgrading to a different version of the rmp_serde library which may have changed the API.

Other changes include:

• In api/src/messages/request.rs, the attribute request is renamed to inner_operations.
• In api/src/messages/response.rs, the metadata attribute is removed from the ResponseTuple struct.
• In src/messages/request.rs, the RequestData struct is removed, and the KeyOperation enum is updated with a comment change.

Potential problems could arise from ensuring that the changes in serialization library behavior (from rmp_serde::to_vec to rmp_serde::to_vec_named) do not affect how the messages are serialized and deserialized.

Additionally, the removal of the metadata attribute in ResponseTuple and the removal of the RequestData struct may have potential impacts on the program's functionality that require thorough testing.

Commit 23

The patch contains changes addressing the Resume PUT & GET to InnerKeyOperation. Key changes and potential problems are as follows:

Key Changes:

1. Two new fields inner_operations and client_operations have been added to the Request struct. inner_operations is used for internal key operations, while client_operations is used for client key operations.
2. A new enum InnerKeyOperation has been introduced which is an abstraction for single key operations, used only within Anna.
3. Added a new field raw_lattice in the ResponseTuple struct to store the lattice value for this key if the operation is an InnerKeyOperation.
4. Refactored code in multiple files to use MixKeyOperation::Inner for inner key operations and MixKeyOperation::Client for client key operations.
5. Updated test cases according to the new changes.

Potential Problems:

1. It is not clear if there could be any compatibility issues with the previous version where inner_operations contained both types of key operations.
2. Ensuring proper handling of errors and edge cases throughout the code with the introduction of new fields and enums.
3. There might be performance implications with the introduction of raw_lattice and updating it for every key operation.
4. It might be necessary to update other parts of the code or tests that depended on the old structure of Request with only inner_operations.

Commit 24

This patch renames KeyOperation to ClientKeyOperation. Here is a summary of the key changes:

• In api/src/messages/request.rs, KeyOperation enum has been renamed to ClientKeyOperation. All instances of KeyOperation have been replaced with ClientKeyOperation.
• In src/messages/request.rs, the same renaming from KeyOperation to ClientKeyOperation has been done.
• Various other files such as src/nodes/client/client_request.rs, src/nodes/client/mod.rs, src/nodes/kvs/handlers/request.rs, src/nodes/kvs/mod.rs, and src/nodes/kvs/store.rs have been updated accordingly to accommodate the renamed ClientKeyOperation.

Potential issues:

• The refactoring seems consistent, but it must be ensured that this renaming does not break any dependencies in other parts of the code that have not been included in the patch.
• Test cases should be re-run to ensure that the renaming does not produce unexpected errors or changes in functionality.

[phil-opp]

Thanks a lot for the updates!

[phil-opp]

We can remove this dependency if it's no longer needed.

[phil-opp]

To avoid the extra allocation, we could replace the self.local_changeset with an empty set:

Suggested change

	for key in self.local_changeset.drain().collect::<Vec<Key>>() {
	for key in std::mem::take(&mut self.local_changeset) {

[phil-opp]

There seems quite a bit of duplication across this new file and src/topics.rs. Given that the anna crate depends on anna-api, it should be possible to reuse the API topics in the anna topics module.

[phil-opp]

Suggested change

	"\n\nValid commands are are GET, PUT, GET_SET, ADD_SET, GET_HASHMAP and ADD_HASHMAP.";
	"\n\nValid commands are are GET, PUT, GET_SET, ADD_SET, GET_HASHMAP, ADD_HASHMAP, and INC.";

[phil-opp]

This silently ignores values that use incorrect syntax. Perhaps it would be better to error instead?

[phil-opp]

Isn't this missing a wait_for_matching_response call?

[phil-opp]

This is still the docstring of MaxLattice.

[phil-opp]

Would it be possible to always go through inner_key_operation_handler? My idea in #4 (comment) was that we could translate the ClientKeyOperation to an InnerKeyOperation that uses the normal merge logic. This way, we could be sure that we don't violate any invariants of the lattice types (e.g. that a value is monotonically increasing).

For example, instead of doing an in-place modification of SingleCausal lattice values, we could construct a new SingleKeyCausalLattice and then invoke inner_key_operation_handler. This way, we also reduce code duplication.

What do you think?

[phil-opp]

This creates an empty vector clock, but if the key already exists, we insert a format!("{}/{}", self.node_id entry into the vector clock (below). Is this intended?

[phil-opp]

Same here.