feat: add get/set to CacheClient to support readme.ts example #185

cprice404 · 2024-03-18T16:43:50Z

The main goal of this PR is to get us closer to having a good example code snippet in the readme.ts file. There were a few things I discovered that we need along the way toward that, so this PR tries to bite off a few of them:

We need get and set, so those are added in this PR.
The credential provider builder pattern didn't seem quite as ergonomic as some of our newer ones (CacheClient/requests), and wasn't consistent with the direction of them, so I tweaked a few things to try to move it in that direction. This is not the final state of this, I intend to do at least one more pass to try to make sure that the builders/constructors for client, requests, cred providers, and configs are as consistent as possible, but this is an interim step.
I also removed/commented out the support for overriding endpoints in the credential providers for now; these had gotten a little stale relative to the current endpoints support, and I'd rather keep the API surface area as small as possible for now. We will probably have to add these back in in some fashion in order to update the CLI to the new release of the SDK but we can do that when we get there.

I will also add a copy paste of what the example code will look like after this PR, but it can't be merged until we do a release because it exists in the examples dir and must be pinned to a released version of the SDK.

The main goal of this PR is to get us closer to having a good example code snippet in the readme.ts file. There were a few things I discovered that we need along the way toward that, so this PR tries to bite off a few of them: * We need `get` and `set`, so those are added in this PR. * The credential provider builder pattern didn't seem quite as ergonomic as some of our newer ones (CacheClient/requests), and wasn't consistent with the direction of them, so I tweaked a few things to try to move it in that direction. This is not the final state of this, I intend to do at least one more pass to try to make sure that the builders/constructors for client, requests, cred providers, and configs are as consistent as possible, but this is an interim step. * I also removed/commented out the support for overriding endpoints in the credential providers for now; these had gotten a little stale relative to the current endpoints support, and I'd rather keep the API surface area as small as possible for now. We will probably have to add these back in in some fashion in order to update the CLI to the new release of the SDK but we can do that when we get there. I will also add a copy paste of what the example code will look like after this PR, but it can't be merged until we do a release because it exists in the examples dir and must be pinned to a released version of the SDK.

cprice404 · 2024-03-18T16:46:19Z

Here's an early example of what the readme.ts example code might look like after these changes:

use std::time::Duration;
use momento::{CacheClient, CredentialProvider};
use momento::config::configurations::laptop;

const CACHE_NAME: String = "cache";

pub async fn main() {
    let cache_client = CacheClient::new(
        CredentialProvider::from_env_var("MOMENTO_API_KEY")?,
        laptop::latest(),
        Duration::from_secs(60)
    )?;

    cache_client.create_cache(CACHE_NAME).await?;

    match(cache_client.set(CACHE_NAME, "mykey", "myvalue").await) {
        Ok(_) => println!("Successfully stored key 'mykey' with value 'myvalue'"),
        Err(e) => println!("Error: {}", e)
    }
    let item: String = match(cache_client.get(&cache_name, "mykey").await?) {
        Get::Hit { value } => value.try_into().expect("I stored a string!"),
        Get::Miss => return Err(anyhow::Error::msg("cache miss")) // probably you'll do something else here
    };

cprice404 · 2024-03-18T16:47:18Z

src/credential_provider.rs

@@ -58,134 +111,102 @@ impl Debug for AuthTokenSource {
 }

 #[derive(Debug)]
-pub struct CredentialProviderBuilder {
+struct CredentialProviderBuilder {


for now I made this builder private. We use it internally, and some parts of it may become public again in the future, but I wanted to simplify the 90% case for creating a CredentialProvider for now.

cprice404 · 2024-03-18T16:48:08Z

src/credential_provider.rs

-    }
-
-    pub fn build(self) -> MomentoResult<CredentialProvider> {
+    // /// Override the data plane endpoint


Here's all the stuff where we were previously allowing endpoint overrides, that I have temporarily commented out. I can delete this stuff instead of commenting it if anyone feels strongly but it felt like it might be useful again in the not-too-distant future.

nand4011 · 2024-03-18T23:29:08Z

src/cache_client.rs

+    /// * `cache_name` - name of cache
+    /// * `key` - key of the item whose value we are setting
+    /// * `value` - data to stored in the cache item
+    /// * `ttl` - The TTL to use for the


Should this be in an optional arguments section?

oh good catch, will fix, thanks

fixed in a82609c , ty

src/credential_provider.rs

src/requests/cache/basic/set.rs

nand4011 · 2024-03-18T23:40:40Z

src/response/cache/sorted_set_fetch.rs

@@ -6,6 +6,8 @@ use momento_protos::cache_client::SortedSetFetchResponse;

 use crate::{MomentoError, MomentoResult};

+// TODO this needs to be moved to the requests directory


I made it a separate file because it is used by multiple requests. Are we going to define all responses alongside the requests because they are closely tied to them?

yeah i meant to reach out to you about this and get your opinion.

tbh part of my motivation here is just to help us in tracking milestones toward the release - my current thinking is that the requests directory will go away entirely. I'd rather not have to surgically check all the files in there and see which to keep and which to get rid of. So I'm thinking maybe we just move all of the stuff that is going to survive over into the requests directory, even though I agree with you that there will be a few files that are shared amongst multiple request types. wdyt?

I made the cache directory in responses and added the sorted set fetch response there, mostly so we could easily separate them. I am alright moving it though, especially since sorted set fetch is one of the only responses that is reused.

maybe we even move it back to responses dir after the old one is deleted, but i think it will be easier to track progress for now if we just plan on getting rid of that whole dir.

Co-authored-by: Nate Anderson <[email protected]>

cprice404 · 2024-03-19T00:35:55Z

@nand4011 I think I fixed everything you found, just the one open question about the plan to move stuff to requests dir.

cprice404 requested a review from nand4011 March 18, 2024 16:43

cprice404 commented Mar 18, 2024

View reviewed changes

nand4011 reviewed Mar 18, 2024

View reviewed changes

cprice404 and others added 3 commits March 18, 2024 16:42

Update src/credential_provider.rs

7e2441b

Co-authored-by: Nate Anderson <[email protected]>

Update src/credential_provider.rs

1cac77c

Co-authored-by: Nate Anderson <[email protected]>

chore: minor cleanups from PR feedback

a82609c

cprice404 requested a review from nand4011 March 19, 2024 00:36

nand4011 approved these changes Mar 19, 2024

View reviewed changes

cprice404 merged commit 8d13be9 into main Mar 19, 2024
6 checks passed

cprice404 deleted the readme-example branch March 19, 2024 03:01

momento-github-actions-machine-user mentioned this pull request Mar 20, 2024

chore(main): release 0.33.0 #188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add get/set to CacheClient to support readme.ts example #185

feat: add get/set to CacheClient to support readme.ts example #185

cprice404 commented Mar 18, 2024

cprice404 commented Mar 18, 2024

cprice404 Mar 18, 2024

cprice404 Mar 18, 2024

nand4011 Mar 18, 2024

cprice404 Mar 18, 2024

cprice404 Mar 19, 2024

nand4011 Mar 18, 2024

cprice404 Mar 18, 2024

nand4011 Mar 19, 2024

cprice404 Mar 19, 2024

cprice404 commented Mar 19, 2024

		@@ -6,6 +6,8 @@ use momento_protos::cache_client::SortedSetFetchResponse;

		use crate::{MomentoError, MomentoResult};

		// TODO this needs to be moved to the requests directory

feat: add get/set to CacheClient to support readme.ts example #185

feat: add get/set to CacheClient to support readme.ts example #185

Conversation

cprice404 commented Mar 18, 2024

cprice404 commented Mar 18, 2024

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

cprice404 commented Mar 19, 2024