Implement Meta Arithmetic functionality #71

mrattle · 2024-10-25T21:26:58Z

TL;DR - What are you trying to accomplish?

This PR introduces meta_arithmetic functionality and does some further refactoring to simplify code after patterns and common code between different methods became more clear.

Details - How are you making this change? What are the effects of this change?

The Meta Arithmetic functionality introduced here covers associated mode switching for incrementing & decrementing, CAS semantics, non-standard deltas and autovivification on nonexistent keys.

mrattle · 2024-10-25T21:46:42Z

src/parser/meta_parser.rs

+pub fn parse_meta_arithmetic_response(
+    buf: &[u8],
+) -> Result<Option<(usize, MetaResponse)>, ErrorKind> {
+    let total_bytes = buf.len();
+    let result = parse_meta_arithmetic_data_value(buf);
+
+    match result {
+        Ok((remaining_bytes, response)) => {
+            let read_bytes = total_bytes - remaining_bytes.len();
+            Ok(Some((read_bytes, response)))
+        }
+        Err(nom::Err::Incomplete(_)) => Ok(None),
+        Err(nom::Err::Error(e)) | Err(nom::Err::Failure(e)) => {
+            Err(ErrorKind::Protocol(Some(e.code.description().to_string())))
+        }
+    }
+}


These parse_meta_*_response methods can probably be refactored into a generic parse_meta_response closure that takes the appropriate value parser for each operation.

mrattle · 2024-10-25T21:50:01Z

src/parser/meta_parser.rs

-            let (input, data) = take_until_size(input, size)?; // parses the data from the input
+            let (input, mut data) = take_until_size(input, size)?; // parses the data from the input
+
+            data = data.trim_ascii_end();


Needed to add this in to compensate for memcached behaviour when decrementing a counter from a number with n digits to a number with n-1 digits. When this happens, the memcached server still holds the same size parameter for the value and fills the extra number of bytes with whitespace when the value is fetched.

e.g. decrementing from 10 -> 9 and then performing a meta_get with v flag on that key will give a value of 9_ (underscore representing whitespace) instead of the expected 9.

…ogic

nickamorim · 2024-10-29T13:53:08Z

src/parser/meta_parser.rs

+        value(MetaResponse::Status(Status::NotFound), tag(b"NF")),
+        value(MetaResponse::Status(Status::NotStored), tag(b"NS")),
+        value(MetaResponse::Status(Status::Exists), tag(b"EX")),
+        value(MetaResponse::Status(Status::NoOp), tag(b"MN\r\n")),


Why do all of the above response codes not contain \r\n and the no-op one does?

If you request the key or an opaque in return then they will be part of the response from the server and the code won't be followed by \r\n. MN is always followed by \r\n.

Does Tag always look at the first two characters? I'm not exactly sure how this is parsing things, was wondering about what happens if the key has HD in it for example.

nom's tag parser will look for the first instance of the specified text exactly, and this is the first parsing step being called. All responses from the server begin with a two character status code so there should never be an opportunity to find an errant HD etc. elsewhere in the response through this step.

nickamorim · 2024-10-29T13:55:29Z

src/parser/meta_parser.rs

+        MetaResponse::Status(s) => process_meta_response_without_data_payload(input, s),
+        _ => Err(nom::Err::Error(nom::error::Error::new(
+            input,
+            nom::error::ErrorKind::Eof,


Good callout, I think this was a holdover from the prototype code. EoF should correspond to a buffer issue during parsing which doesn't fit here so I'm going to replace with Fail, which is the most generic nom error code available. The IResult in the return value requires that the error type be par of nom::error::ErrorKind:: which is a bit annoying.

src/proto/meta_protocol.rs

nickamorim · 2024-10-29T13:57:51Z

src/proto/meta_protocol.rs

+    // Mode switching between increment and decrement is done with the M flag.
+    // The default behaviour is to increment with a delta of 1.
+    //
+    // Using the "q" flag for quiet mode will append a no-op command to the request ("mn\r\n") so that the client


so that the client can proceed properly in the event of a cache miss.

Using a q flag just means that the client doesn't wait for the response so what does that have to do with a cache miss?

Errors are still returned from the memcached server when the q flag is used. For Meta Get, EN responses (cache miss) are suppressed, for Meta Set, HD responses (success) are suppressed etc. We still have to use drive_recieve to pull responses off the buffer and if they're left unmanaged then the following command can parse incorrect information, unless we clear the buffer before writing every command.

The errors do have meaning and would impact client logic down the line in the case of something like a CAS operation, so I don't really think that they can just be safely ignored.

nickamorim · 2024-10-29T13:58:49Z

src/proto/meta_protocol.rs

+    fn write_non_set_command(
+        &mut self,
+        op: &str,
+        kr: &[u8],


Suggested change

kr: &[u8],

fn write_non_set_command<K: AsRef<[u8]>>(

&mut self,

op: &str,

key: K,

I'm also not a huge fan of this abstraction. I'd prefer just to explicitly keep the logic, even though I recognize it's duplicated, in each of the respective methods

Fair, it's possible that there might be different optimizations to be made for each different op. I'll walk this back.

Yah, also write_non_set command feels a bit wrong to name it like this. I think having seperate instances is probably correct to start.

nickamorim · 2024-10-29T14:09:44Z

src/proto/meta_protocol.rs

@@ -200,4 +212,52 @@ impl MetaProtocol for Client {
                .transpose(),
        }
    }
+
+    async fn meta_arithmetic<K: AsRef<[u8]>>(


Should we make this more convenient for the end-user by adding another "value" arg? And maybe providing an explicitly increment/decrement instead?

We have been pushing down the route of just taking a set of flags so far but I do think that this might be a more compelling argument for explicitly taking params.

Could make this change to take a delta (with default of 1) and decrement as a boolean (default of false, as per the proto). Other flags that should maybe be explicit params are q (so we don't have to search for it) and opaque.

I have been thinking about this in the context of multi-ops as well where you would want to force the use of either an opaque or the k flag so that we can get away from this "trusting that responses are returned in order" issue that we have with that ASCII proto. Having an opaque as a param makes this more straightforward.

We talked about this IRL and decided that a good path would be exposing explicit meta_{increment|decrement} methods that handle adding the respective flags for each method. These can just call meta_arithmetic (private method) under the hood with the correct flags.

grcooper · 2024-10-29T18:14:03Z

src/parser/meta_parser.rs

+        value(MetaResponse::Status(Status::NotFound), tag(b"NF")),
+        value(MetaResponse::Status(Status::NotStored), tag(b"NS")),
+        value(MetaResponse::Status(Status::Exists), tag(b"EX")),
+        value(MetaResponse::Status(Status::NoOp), tag(b"MN\r\n")),


Does Tag always look at the first two characters? I'm not exactly sure how this is parsing things, was wondering about what happens if the key has HD in it for example.

grcooper · 2024-10-29T18:15:58Z

src/proto/meta_protocol.rs

+    fn write_non_set_command(
+        &mut self,
+        op: &str,
+        kr: &[u8],


Yah, also write_non_set command feels a bit wrong to name it like this. I think having seperate instances is probably correct to start.

grcooper · 2024-10-29T18:17:07Z

src/proto/meta_protocol.rs

+        }
+    }
+
+    async fn write_non_set_command(


Is there something that groups this that isn't a negative? Like all retrieval commands? I think that is how memcached organizes them https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L62

I agree, non_set is just awkward verbiage, and retrieval feels slightly better but it still isn't quite right in this context since it's doing more than just a fetch. The real differentiator is just the lack of a data block / value payload being written, which only happens in the case of meta_set.

write_meta_command_without_payload?

Actually, nevermind. Irrelevant if I'm walking that bit of code back anyway 😅 .

nickamorim

Approving but pls take a look at some of the comments/questions

nickamorim · 2024-11-04T17:22:54Z

src/proto/meta_protocol.rs

        &mut self,
        key: K,
+        is_quiet: bool,
+        opaque: Option<&[u8]>,
+        delta: Option<u64>,
        meta_flags: Option<&[&str]>,


Should we validate that a MI isn't passed in because I'm assuming Memcached will throw up if you pass in duplicate flags?

Yeah, memcached has a bad time if there are duplicate flags.

When checking the flags, options around this are to either:

Raise an exception when a flag with an explicit param is used (which would lead to duplication)

Ignore flags that should be passed in as an explicit param (could lead to unexpected behaviour on the user's end)

Prefer explicit params over raw meta flags (i.e. use param and reject flag if both are mistakenly provided), but use raw meta flags if provided in lieu of using the optional params

Last option is easy enough to implement and is the most flexible / accomodating, and doesn't just nuke a command. All options require iteration over the &[&str] flags provided, so I don't think there is much of a perf diff either way.

src/proto/meta_protocol.rs

tests/meta_proto_integration_tests.rs

…gnature

…r problematic meta flag duplication

mrattle self-assigned this Oct 25, 2024

mrattle changed the base branch from main to mrattle/implement-meta-delete October 25, 2024 21:27

mrattle force-pushed the mrattle/implement-meta-arithmetic branch from 861efde to 2f77099 Compare October 25, 2024 21:37

mrattle marked this pull request as ready for review October 25, 2024 21:41

mrattle requested a review from a team as a code owner October 25, 2024 21:41

mrattle commented Oct 25, 2024

View reviewed changes

mrattle force-pushed the mrattle/implement-meta-delete branch 2 times, most recently from 90bcd09 to 447f4bb Compare October 28, 2024 19:35

Base automatically changed from mrattle/implement-meta-delete to main October 28, 2024 19:37

mrattle added 4 commits October 28, 2024 15:53

Stub out meta arithmetic code, extract non-set command construction l…

ed830db

…ogic

complete arithmetic method and add tests

a63a3f3

CHANGELOG

1a444df

rebase to current state of main

c836405

mrattle force-pushed the mrattle/implement-meta-arithmetic branch from 4caf277 to c836405 Compare October 28, 2024 20:22

nickamorim reviewed Oct 29, 2024

View reviewed changes

grcooper reviewed Oct 29, 2024

View reviewed changes

addressing PR comments

841220f

mrattle force-pushed the mrattle/implement-meta-arithmetic branch from fa63d5e to 841220f Compare October 31, 2024 20:37

grcooper approved these changes Nov 1, 2024

View reviewed changes

nickamorim approved these changes Nov 4, 2024

View reviewed changes

mrattle added 2 commits November 4, 2024 13:11

swap meta_arithmetic for specific incr/decr methods and modify API si…

59eb2b1

…gnature

add custom error type for opaque length

15c38d6

mrattle force-pushed the mrattle/implement-meta-arithmetic branch from b08ffa1 to 9d4b21b Compare November 4, 2024 21:53

mrattle added 2 commits November 4, 2024 17:16

prefer explicit params over meta flags and eliminate opportunities fo…

cc43db4

…r problematic meta flag duplication

add test for opaque validation, general cleanup

ead4f76

mrattle force-pushed the mrattle/implement-meta-arithmetic branch from a6be0a6 to ead4f76 Compare November 4, 2024 22:16

mrattle merged commit 1d47c0a into main Nov 4, 2024
4 checks passed

mrattle deleted the mrattle/implement-meta-arithmetic branch November 4, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Meta Arithmetic functionality #71

Implement Meta Arithmetic functionality #71

mrattle commented Oct 25, 2024 •

edited

Loading

mrattle Oct 25, 2024

mrattle Oct 25, 2024

nickamorim Oct 29, 2024

mrattle Oct 29, 2024

grcooper Oct 29, 2024

mrattle Oct 31, 2024

nickamorim Oct 29, 2024

mrattle Oct 31, 2024

nickamorim Oct 29, 2024

mrattle Oct 31, 2024

nickamorim Oct 29, 2024

nickamorim Oct 29, 2024

mrattle Oct 29, 2024

grcooper Oct 29, 2024

nickamorim Oct 29, 2024

mrattle Oct 31, 2024 •

edited

Loading

nickamorim Oct 31, 2024

grcooper Oct 29, 2024

grcooper Oct 29, 2024

grcooper Oct 29, 2024

mrattle Oct 31, 2024

mrattle Oct 31, 2024

nickamorim left a comment

nickamorim Nov 4, 2024

mrattle Nov 4, 2024

Implement Meta Arithmetic functionality #71

Implement Meta Arithmetic functionality #71

Conversation

mrattle commented Oct 25, 2024 • edited Loading

TL;DR - What are you trying to accomplish?

Details - How are you making this change? What are the effects of this change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrattle Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickamorim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrattle commented Oct 25, 2024 •

edited

Loading

mrattle Oct 31, 2024 •

edited

Loading