Allow `set` and `add` methods to take uint types in addition to &str #28

mrattle · 2024-08-30T02:57:11Z

Closes: https://github.com/Shopify/caching-platform/issues/694

The original implementation of the set and add methods did not allow integer inputs, which doesn't make sense in the context of arithmetic methods like increment and decrement.

This PR introduces an AsMemcachedValue trait that serializes value inputs of different types into bytes to be used in memcached commands.

src/parser/mod.rs

mrattle · 2024-08-30T17:23:31Z

src/parser/mod.rs

+impl_to_memcached_value_for_uint!(u8);
+impl_to_memcached_value_for_uint!(u16);
+impl_to_memcached_value_for_uint!(u32);
+impl_to_memcached_value_for_uint!(u64);
+impl_to_memcached_value_for_uint!(usize);


I only covered uints up to u64 here because afaik we should not be initializing negative counters, and u64 is the largest size of uint that SFR is expecting.

danmayer · 2024-09-03T14:57:02Z

@mrattle some conflicts need to be resolved

danmayer

looks good

…s for value, cleanup tests

adampetro · 2024-09-03T16:11:53Z

src/parser/mod.rs

@@ -103,6 +109,67 @@ pub struct KeyMetadata {
    pub size: u32,
 }

+/// A trait for parsing multiple types of values to memcached values.


Why is this in parser/mod.rs? Isn't this only used for serializing?

You're right, doesn't quite make sense in there. Chopped it out into it's own file to try to avoid bloat in lib.rs.

adampetro · 2024-09-03T16:13:50Z

src/parser/mod.rs

+    fn length(&self) -> usize;
+    /// Writes the value to a writer.
+    fn write_to<W: AsyncWriteExt + Unpin>(
+        &self,
+        writer: &mut W,
+    ) -> impl Future<Output = Result<(), crate::Error>>;


Could we collapse this into an fn as_bytes(&self) -> &[u8] method? (Or maybe returning std::borrow::Cow<'_, [u8]> if we need the option of returning owned values). This way we could avoid writing the uint types to the buffer twice and do so only once

Thanks for the suggestion, I was actually trying to work out a way to do exactly that so I'll play around with it a bit more.

I'm running in to a couple of issues when trying to return bytes (either directly or through a Cow<_>:

for -> &[u8] - trying to return an owned value and there doesn't seem to be a concise way to satisfy the compiler

for -> Cow<'_, [u8]>: Cow is looking for a vector which requires another allocation, but also seems to just return a reference to empty memory since the underlying data is dropped before it's used.

The path of least resistance to satisfy the compiler is to return a Vec<u8> but this is very slow for long strings so it's a non-starter. I agree that parsing as bytes / writing to a buffer more than once is not great if it can be avoided but any performance cost seems to be negligible compared to a lot of the functional alternatives, though I could definitely be missing something.

adampetro · 2024-09-03T16:23:40Z

src/parser/mod.rs

@@ -103,6 +109,67 @@ pub struct KeyMetadata {
    pub size: u32,
 }

+/// A trait for parsing multiple types of values to memcached values.
+pub trait ToMemcachedValue {


Is the intention that we want library consumers to be able to implement this on their own types? My preference would be to not allow that unless there is a need, and to prevent that with a private supertrait as described here.

Something like:

mod private { pub(super) trait ToMemcachedValue { ... } } pub trait ToMemcachedValue: private::ToMemcachedValue { ... }

Thanks, I've refactored this with the sealed trait pattern.

adampetro · 2024-09-03T16:25:22Z

src/parser/mod.rs

+    ) -> impl Future<Output = Result<(), crate::Error>>;
+}
+
+impl ToMemcachedValue for &[u8] {


Can we do this so that we can continue to support all types that we did previously

Suggested change

impl ToMemcachedValue for &[u8] {

impl<T: AsRef<[u8]>> ToMemcachedValue for T {

When going down this route I end up having to create a standalone wrapper struct for u64 inputs which sort of defeats the intention behind this change, since it ends up making it more awkward to use a uint value.

I've included String and &String implementations for ToMemcachedValue which I think covers everything from AsRef that we would realistically see as input for the set and add functions here. For reference, this rust-memcache crate is where we picked up this pattern from and they have implemented on the same set of types that I have updated this PR to include.

…sealed trait pattern to prevent external impl on consumer types

mrattle added the #gsd:42391 Meta protocol in async-memcached label Aug 30, 2024

mrattle self-assigned this Aug 30, 2024

mrattle mentioned this pull request Aug 30, 2024

Modify set method trait bounds to additionally accept u64 parameter #27

Closed

nickamorim approved these changes Aug 30, 2024

View reviewed changes

src/parser/mod.rs Outdated Show resolved Hide resolved

mrattle changed the title ~~Allow set method to take uint types in addition to &str~~ Allow set and add methods to take uint types in addition to &str Aug 30, 2024

mrattle commented Aug 30, 2024

View reviewed changes

mrattle force-pushed the mrattle/tomemcachedvalue-for-set branch from 8d22dc8 to e2da127 Compare September 3, 2024 12:53

mrattle marked this pull request as ready for review September 3, 2024 13:28

mrattle requested a review from a team as a code owner September 3, 2024 13:28

mrattle requested review from a team, makrisoft and mkcny and removed request for a team September 3, 2024 13:49

mrattle force-pushed the mrattle/tomemcachedvalue-for-set branch from 8ce7e23 to e1844e4 Compare September 3, 2024 13:53

danmayer approved these changes Sep 3, 2024

View reviewed changes

mrattle force-pushed the mrattle/tomemcachedvalue-for-set branch from e1844e4 to ca43669 Compare September 3, 2024 15:11

mrattle added 4 commits September 3, 2024 11:17

add ToMemcachedValue trait and impl for required types

98721f6

rework set method using ToMemcachedValue to allow many parameter type…

a056662

…s for value, cleanup tests

fix benchmarks, add more to compare new and old set functions

7280c46

rename len method to length to satisfy linter

ace7f6f

mrattle force-pushed the mrattle/tomemcachedvalue-for-set branch from ca43669 to 605fc21 Compare September 3, 2024 15:22

drinkbeer approved these changes Sep 3, 2024

View reviewed changes

adampetro reviewed Sep 3, 2024

View reviewed changes

mrattle and others added 6 commits September 5, 2024 16:36

allow uint value argumenton add method, update changelog

a803502

cleanup old set method and related code + tests

c6e1583

move ToMemcachedValue from parser/mod to value_serializer.rs and use …

e33a5a4

…sealed trait pattern to prevent external impl on consumer types

increase ToMemcachedValue type coverage to include String and &String

c37dbdc

Prototyping

2d79ede

benchmarks for add methods and cleanup

c4c9eb0

mrattle force-pushed the mrattle/tomemcachedvalue-for-set branch from 17af8e7 to c4c9eb0 Compare September 5, 2024 20:37

adampetro approved these changes Sep 5, 2024

View reviewed changes

mrattle merged commit fad4077 into main Sep 5, 2024
4 checks passed

mrattle deleted the mrattle/tomemcachedvalue-for-set branch September 5, 2024 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `set` and `add` methods to take uint types in addition to &str #28

Allow `set` and `add` methods to take uint types in addition to &str #28

mrattle commented Aug 30, 2024 •

edited

Loading

mrattle Aug 30, 2024

danmayer commented Sep 3, 2024

danmayer left a comment

adampetro Sep 3, 2024

mrattle Sep 4, 2024

adampetro Sep 3, 2024

mrattle Sep 3, 2024

mrattle Sep 5, 2024 •

edited

Loading

adampetro Sep 3, 2024

mrattle Sep 4, 2024

adampetro Sep 3, 2024

mrattle Sep 5, 2024

	impl ToMemcachedValue for &[u8] {
	impl<T: AsRef<[u8]>> ToMemcachedValue for T {

Allow set and add methods to take uint types in addition to &str #28

Allow set and add methods to take uint types in addition to &str #28

Conversation

mrattle commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

danmayer commented Sep 3, 2024

danmayer left a 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 Sep 5, 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

Allow `set` and `add` methods to take uint types in addition to &str #28

Allow `set` and `add` methods to take uint types in addition to &str #28

mrattle commented Aug 30, 2024 •

edited

Loading

mrattle Sep 5, 2024 •

edited

Loading