Skip to content

feat: momento apis - set_if_absent, set_if_present_and_not_equal, item_get_type #372

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

Open
nidhidhamnani wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: momento apis - set_if_absent, set_if_present_and_not_equal, item_get_type #372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9744b9a
feat: set_if api
nidhidhamnani Mar 27, 2025
745e913
chore: remove .idea files
nidhidhamnani Mar 27, 2025
daf8ea2
feat: set_if_present_and_not_equal api and item_get_type api
nidhidhamnani Mar 28, 2025
4803565
fix: PR feedbacks
nidhidhamnani Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/clients/cache/momento/commands/item_get_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use super::*;
use ::momento::cache::ItemGetTypeResponse;

pub async fn item_get_type(
client: &mut CacheClient,
config: &Config,
cache_name: &str,
request: workload::client::ItemGetType,
) -> std::result::Result<(), ResponseError> {
ITEM_GET_TYPE.increment();

match timeout(
config.client().unwrap().request_timeout(),
client.item_get_type(cache_name, (*request.key).to_owned()),
)
.await
{
Ok(Ok(r)) => match r {
ItemGetTypeResponse::Hit { .. } => {
ITEM_GET_TYPE_HIT.increment();
Ok(())
}
ItemGetTypeResponse::Miss => {
ITEM_GET_TYPE_MISS.increment();
Ok(())
}
},
Ok(Err(e)) => {
ITEM_GET_TYPE_EX.increment();
Err(e.into())
}
Err(_) => {
ITEM_GET_TYPE_TIMEOUT.increment();
Err(ResponseError::Timeout)
}
}
}
6 changes: 6 additions & 0 deletions src/clients/cache/momento/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ mod hash_get;
mod hash_get_all;
mod hash_increment;
mod hash_set;
mod item_get_type;
mod list_fetch;
mod list_length;
mod list_pop_back;
Expand All @@ -18,6 +19,8 @@ mod list_push_front;
mod list_remove;
mod set;
mod set_add;
mod set_if_absent;
mod set_if_present_and_not_equal;
mod set_members;
mod set_remove;
mod sorted_set_add;
Expand All @@ -34,6 +37,7 @@ pub use hash_get::*;
pub use hash_get_all::*;
pub use hash_increment::*;
pub use hash_set::*;
pub use item_get_type::*;
pub use list_fetch::*;
pub use list_length::*;
pub use list_pop_back::*;
Expand All @@ -43,6 +47,8 @@ pub use list_push_front::*;
pub use list_remove::*;
pub use set::*;
pub use set_add::*;
pub use set_if_absent::*;
pub use set_if_present_and_not_equal::*;
pub use set_members::*;
pub use set_remove::*;
pub use sorted_set_add::*;
Expand Down
41 changes: 41 additions & 0 deletions src/clients/cache/momento/commands/set_if_absent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
use super::*;
use ::momento::cache::SetIfAbsentResponse;

pub async fn set_if_absent(
client: &mut CacheClient,
config: &Config,
cache_name: &str,
request: workload::client::SetIfAbsent,
) -> std::result::Result<(), ResponseError> {
SET_IF_ABSENT.increment();

match timeout(
config.client().unwrap().request_timeout(),
client.set_if_absent(
cache_name,
(*request.key).to_owned(),
(*request.value).to_owned(),
),
)
.await
Copy link
Copy Markdown
Contributor

@krispraws krispraws Mar 28, 2025

Choose a reason for hiding this comment

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

I wonder if we could move some of this boilerplate code into record_result!. Especially for conditional write responses where we do not care about the actual value of the item in the response and just want to record metrics for stored vs not_stored. The macro already seems to have some logic based on 3 arguments being passed to it

Copy link
Copy Markdown
Contributor

@brayniac brayniac Mar 31, 2025

Choose a reason for hiding this comment

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

I think you could, but it'd need enough other code to support inspecting arbitrary enums to check stored/not_stored (some sort of trait that's implemented for each conditional write response enum?) that I don't know if it'd save any real code.

record_result!() could maybe still be used here for the EX and TIMEOUT - it'd expect to increment something like SET_IF_ABSENT_OK - which should exist since the request! macro does some paste magic to generate _OK, _EX, and _TIMEOUT metrics for the same base request type. Then this could be a record_result!() and an

record_result!(...);

if let Ok(Ok(r)) = r { 
    match r { 
        /// enum variant handling 
    }
 }

Might be worth making that change

{
Ok(Ok(r)) => match r {
SetIfAbsentResponse::Stored { .. } => {
SET_IF_ABSENT_STORED.increment();
Ok(())
}
SetIfAbsentResponse::NotStored => {
SET_IF_ABSENT_NOT_STORED.increment();
Ok(())
}
},
Ok(Err(e)) => {
SET_IF_ABSENT_EX.increment();
Err(e.into())
}
Err(_) => {
SET_IF_ABSENT_TIMEOUT.increment();
Err(ResponseError::Timeout)
}
}
}
42 changes: 42 additions & 0 deletions src/clients/cache/momento/commands/set_if_present_and_not_equal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use super::*;
use ::momento::cache::SetIfPresentAndNotEqualResponse;

pub async fn set_if_present_and_not_equal(
client: &mut CacheClient,
config: &Config,
cache_name: &str,
request: workload::client::SetIfPresentAndNotEqual,
) -> std::result::Result<(), ResponseError> {
SET_IF_PRESENT_AND_NOT_EQUAL.increment();

match timeout(
config.client().unwrap().request_timeout(),
client.set_if_present_and_not_equal(
cache_name,
(*request.key).to_owned(),
(*request.new_value).to_owned(),
(*request.old_value).to_owned(),
),
)
.await
{
Ok(Ok(r)) => match r {
Copy link
Copy Markdown
Contributor

@brayniac brayniac Mar 31, 2025

Choose a reason for hiding this comment

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

Same here as with my comment above. record_result + if let might make this cleaner)

SetIfPresentAndNotEqualResponse::Stored { .. } => {
SET_IF_PRESENT_AND_NOT_EQUAL_STORED.increment();
Ok(())
}
SetIfPresentAndNotEqualResponse::NotStored => {
SET_IF_PRESENT_AND_NOT_EQUAL_NOT_STORED.increment();
Ok(())
}
},
Ok(Err(e)) => {
SET_IF_PRESENT_AND_NOT_EQUAL_EX.increment();
Err(e.into())
}
Err(_) => {
SET_IF_PRESENT_AND_NOT_EQUAL_TIMEOUT.increment();
Err(ResponseError::Timeout)
}
}
}
9 changes: 9 additions & 0 deletions src/clients/cache/momento/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,15 @@ async fn task(
ClientRequest::Get(r) => get(&mut client, &config, cache_name, r).await,
ClientRequest::Set(r) => set(&mut client, &config, cache_name, r).await,
ClientRequest::Delete(r) => delete(&mut client, &config, cache_name, r).await,
ClientRequest::SetIfAbsent(r) => {
set_if_absent(&mut client, &config, cache_name, r).await
}
ClientRequest::SetIfPresentAndNotEqual(r) => {
set_if_present_and_not_equal(&mut client, &config, cache_name, r).await
}
ClientRequest::ItemGetType(r) => {
item_get_type(&mut client, &config, cache_name, r).await
}

/*
* HASHES (DICTIONARIES)
Expand Down
14 changes: 14 additions & 0 deletions src/config/workload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,20 @@ pub enum Verb {
/// * Momento: unsupported
/// * RESP: `SET` with `XX` option
Replace,
/// Set the value for a key only if it does not exists.
/// * Momento: `set_if_absent`
/// * RESP: `SET` with `XX` option
SetIfAbsent,
/// Set the value for a key only if it already exists and the
/// current value does not match the value to compare against
/// from the request
/// * Momento: `set_if_present_and_not_equal`
/// * RESP: Raw `SET` with cached value comparison
SetIfPresentAndNotEqual,
/// Get the type of the value stored under the key.
/// * Momento: `item_get_type`
/// * RESP: `TYPE`
ItemGetType,

/*
* HASHES (DICTIONARIES)
Expand Down
20 changes: 19 additions & 1 deletion src/metrics/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ macro_rules! request {
paste! {
#[allow(dead_code)]
pub static [<$ident _EX_COUNTER>]: &'static str = concat!($name, "/exception");
}
}
}

paste! {
Expand Down Expand Up @@ -524,6 +524,24 @@ counter!(
"delete requests that resulted in timeout"
);

request!(SET_IF_ABSENT, "set_if_absent");
Comment thread
nidhidhamnani marked this conversation as resolved.
counter!(SET_IF_ABSENT_STORED, "set_if_absent/stored");
counter!(SET_IF_ABSENT_NOT_STORED, "set_if_absent/not_stored");

request!(SET_IF_PRESENT_AND_NOT_EQUAL, "set_if_present_and_not_equal");
counter!(
SET_IF_PRESENT_AND_NOT_EQUAL_STORED,
"set_if_present_and_not_equal/stored"
);
counter!(
SET_IF_PRESENT_AND_NOT_EQUAL_NOT_STORED,
"set_if_present_and_not_equal/not_stored"
);

request!(ITEM_GET_TYPE, "item_get_type");
counter!(ITEM_GET_TYPE_HIT, "item_get_type/hit");
counter!(ITEM_GET_TYPE_MISS, "item_get_type/miss");

request!(HASH_GET, "hash_get");
counter!(HASH_GET_FIELD_HIT, "hash_get/field_hit");
counter!(HASH_GET_FIELD_MISS, "hash_get/field_miss");
Expand Down
21 changes: 21 additions & 0 deletions src/workload/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,24 @@ pub struct Set {
pub ttl: Option<Duration>,
}

#[derive(Debug, PartialEq)]
pub struct SetIfAbsent {
pub key: Arc<[u8]>,
pub value: Bytes,
}

#[derive(Debug, PartialEq)]
pub struct SetIfPresentAndNotEqual {
Comment thread
nidhidhamnani marked this conversation as resolved.
pub key: Arc<[u8]>,
pub new_value: Bytes,
pub old_value: Bytes,
}

#[derive(Debug, PartialEq)]
pub struct ItemGetType {
pub key: Arc<[u8]>,
}

// Hash

#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -212,6 +230,9 @@ pub enum ClientRequest {
MultiGet(MultiGet),
Replace(Replace),
Set(Set),
SetIfAbsent(SetIfAbsent),
SetIfPresentAndNotEqual(SetIfPresentAndNotEqual),
ItemGetType(ItemGetType),

// Hash Commands
HashExists(HashExists),
Expand Down
14 changes: 14 additions & 0 deletions src/workload/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,20 @@ impl Generator {
Verb::Delete => ClientRequest::Delete(client::Delete {
key: keyspace.sample(rng),
}),
Verb::SetIfAbsent => ClientRequest::SetIfAbsent(client::SetIfAbsent {
key: keyspace.sample(rng),
value: keyspace.gen_value(sequence as _, rng),
}),
Verb::SetIfPresentAndNotEqual => {
ClientRequest::SetIfPresentAndNotEqual(client::SetIfPresentAndNotEqual {
key: keyspace.sample(rng),
new_value: keyspace.gen_value(sequence as _, rng),
old_value: keyspace.gen_value(sequence as _, rng),
Copy link
Copy Markdown
Contributor

@krispraws krispraws Mar 28, 2025

Choose a reason for hiding this comment

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

If you generate different values for old_value and new_value, the result will always be stored. Is that intentional?

Copy link
Copy Markdown
Contributor

@brayniac brayniac Mar 31, 2025

Choose a reason for hiding this comment

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

There might not be a good alternative. We don't really have any place we're keeping history for each key to do some % of overwrite. Similar problem as testing something like a CAS operation. You might be able to fake something with two keyspaces? But the workload generation isn't really designed with this in mind.

If there's need in the future, what I'd consider is some mechanism for playing workload from a file and trying to make that flexible in terms of whether exact or random out of N values are used in each place. Then you could dial in some very specific patterns by generating that file or getting some prod trace and replaying it.

For now, maybe a comment acknowledging this behavior and that it's expected would be good?

Copy link
Copy Markdown
Contributor

@krispraws krispraws Apr 1, 2025

Choose a reason for hiding this comment

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

sounds good

})
}
Verb::ItemGetType => ClientRequest::ItemGetType(client::ItemGetType {
key: keyspace.sample(rng),
}),
Verb::Replace => ClientRequest::Replace(client::Replace {
key: keyspace.sample(rng),
value: keyspace.gen_value(sequence as _, rng),
Expand Down
Loading