cashubtc/cdk
1
0
Fork 0
Code Issues 25 Pull requests 23 Projects Releases 43 Packages Wiki Activity Actions

Introduce pluggable backend cache for the HTTP layer. #495

Merged
crodas merged 7 commits from cache-with-custom-backend into main 2024-12-17 12:39:03 +00:00
Conversation 10 Commits 7 Files changed 22 +900 -639
crodas commented 2024-12-07 19:47:05 +00:00 (Migrated from github.com)
Copy link

Description

Fixes #478.

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

Add support for custom storages for the HTTP cache layer.

REMOVED

FIXED

Checklist

### Description Fixes #478. ----- ### Notes to the reviewers <!-- In this section you can include notes directed to the reviewers, like explaining why some parts of the PR were done in a specific way --> ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates #### CHANGED #### ADDED Add support for custom storages for the HTTP cache layer. #### REMOVED #### FIXED ---- ### Checklist * [x] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [x] I ran `just final-check` before committing
crodas commented 2024-12-07 19:47:25 +00:00 (Migrated from github.com)
Copy link

Right now, I have implemented two storage systems, Memory and Redis. I think a 3rd option would be useful as well, a mix of both, where Redis is used to synchronize local cache between N nodes, but the caching is always local. I can work on a prototype if this sounds like a good idea. /cc @callebtc @thesimplekid

Right now, I have implemented two storage systems, Memory and Redis. I think a 3rd option would be useful as well, a mix of both, where Redis is used to synchronize local cache between N nodes, but the caching is always local. I can work on a prototype if this sounds like a good idea. /cc @callebtc @thesimplekid
crodas commented 2024-12-09 19:46:56 +00:00 (Migrated from github.com)
Copy link

I made redis optional now @thesimplekid

I made redis optional now @thesimplekid
thesimplekid (Migrated from github.com) requested changes 2024-12-10 23:02:35 +00:00
thesimplekid (Migrated from github.com) left a comment
Copy link

I think we just need to add docs for how to run redis or at least note it must be run externally and link to something. Maybe add it to docker compose?

Other then that LGTM

I think we just need to add docs for how to run redis or at least note it must be run externally and link to something. Maybe add it to docker compose? Other then that LGTM
crates/cdk-mintd/example.config.toml
@ -6,6 +6,14 @@ mnemonic = ""
# input_fee_ppk = 0
thesimplekid (Migrated from github.com) commented 2024-12-10 22:37:04 +00:00
Copy link
# memory or redis
backend = "memory"

Since the default is memory that should be the default in the example

```suggestion # memory or redis backend = "memory" ``` Since the default is memory that should be the default in the example
thesimplekid (Migrated from github.com) commented 2024-12-10 22:40:27 +00:00
Copy link
# `key_prefix` and `connection_string` required for redis
# key_prefix = "mintd"
# connection_string = "redis://localhost"
```suggestion # `key_prefix` and `connection_string` required for redis # key_prefix = "mintd" # connection_string = "redis://localhost" ```
thesimplekid (Migrated from github.com) requested changes 2024-12-12 10:33:04 +00:00
thesimplekid (Migrated from github.com) left a comment
Copy link

I added a redis container to the docker-compose and the ability to read the cache config from env vars. But @crodas can you take a look at what is causing the panic I noted.

I added a redis container to the docker-compose and the ability to read the cache config from env vars. But @crodas can you take a look at what is causing the panic I noted.
crates/cdk-axum/src/cache/mod.rs
@ -0,0 +1,188 @@
//! HTTP cache.
thesimplekid (Migrated from github.com) commented 2024-12-12 10:31:40 +00:00
Copy link

This is causing a runtime panic when run with redis.

I added a redis container to the docker compose and the ability to read the redis config from env vars. However I'm getting a runtime panic when using redis

image

This is causing a runtime panic when run with redis. I added a redis container to the docker compose and the ability to read the redis config from env vars. However I'm getting a runtime panic when using redis ![image](https://github.com/user-attachments/assets/0d8daecd-585d-4b6f-933a-09a36558ee38)
thesimplekid (Migrated from github.com) reviewed 2024-12-12 11:37:04 +00:00
crates/cdk-mintd/src/config.rs
@ -210,7 +211,10 @@ pub struct MintInfo {
thesimplekid (Migrated from github.com) commented 2024-12-12 11:37:04 +00:00
Copy link

NIT code style should use where https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#generics.

This applies in a few places but ill just comment here. Won't block merging for it

NIT code style should use where https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#generics. This applies in a few places but ill just comment here. Won't block merging for it
crodas (Migrated from github.com) reviewed 2024-12-14 00:13:22 +00:00
crates/cdk-mintd/src/config.rs
@ -210,7 +211,10 @@ pub struct MintInfo {
crodas (Migrated from github.com) commented 2024-12-14 00:13:22 +00:00
Copy link

I think I addressed all the issues in 6a96e9d

I think I addressed all the issues in 6a96e9d
crodas (Migrated from github.com) reviewed 2024-12-14 00:14:44 +00:00
crates/cdk-axum/src/cache/mod.rs
@ -0,0 +1,188 @@
//! HTTP cache.
crodas (Migrated from github.com) commented 2024-12-14 00:14:44 +00:00
Copy link

I removed all instances of unwrap, but I could not reproduce them. Your Redis server seems to be in a bad state. Can you please pull all my latest and try again? Please paste here anything weird with the logs regarding cache

Cheers,

I removed all instances of unwrap, but I could not reproduce them. Your Redis server seems to be in a bad state. Can you please pull all my latest and try again? Please paste here anything weird with the logs regarding cache Cheers,
thesimplekid (Migrated from github.com) requested changes 2024-12-16 18:45:54 +00:00
crates/cdk-axum/src/cache/backend/redis.rs
@ -0,0 +1,96 @@
use std::time::Duration;
thesimplekid (Migrated from github.com) commented 2024-12-16 18:45:50 +00:00
Copy link

It seems redis returns an empty vec if there is no value for a key. This is causeing the deserlization error we've talked about offline. If we just check if the vec is empty here and return None if it is I believe this PR is good to merge.

It seems redis returns an empty vec if there is no value for a key. This is causeing the deserlization error we've talked about offline. If we just check if the vec is empty here and return None if it is I believe this PR is good to merge.
crodas (Migrated from github.com) reviewed 2024-12-16 21:00:52 +00:00
crates/cdk-axum/src/cache/backend/redis.rs
@ -0,0 +1,96 @@
use std::time::Duration;
crodas (Migrated from github.com) commented 2024-12-16 21:00:52 +00:00
Copy link

@thesimplekid I forgot the ?, but I think a26bb7ad4b is more readable

@thesimplekid I forgot the `?`, but I think a26bb7ad4b036a4f27f3d71d6e62c2514ba8f193 is more readable
thesimplekid (Migrated from github.com) approved these changes 2024-12-17 12:38:38 +00:00
thesimplekid (Migrated from github.com) left a comment
Copy link

Thanks, LGTM

ACK 8f3104f489

Thanks, LGTM ACK 8f3104f489034d28fbee7d6688147c6c399dd910
Sign in to join this conversation.
Reviewers
No reviewers
thesimplekid
Labels
Clear labels   
DB & Storage

   
Deployment

   
Error Handling & Logging

   
Maintenance

   
Payment Backend

   
backport

   
backport v0.13.x

   
backport v0.14.x

   
backport v0.15.x

   
bindings

   
blocked

   
bug

Something isn't working

  
cdk-sql

   
ci

   
cli

   
deps

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
keep-open

   
ldk-node-ui

   
migrations

   
mint

   
mutation-testing

   
needs rebase

   
needs review

   
new nut

This PR is held pending the merge of a new or updated NUT

  
nut change

   
question

Further information is requested

  
ready

   
rust-version

   
rustfmt

   
stacked hold

PR is based on top of another PR, so should be merged in sequence. See comments for details.

  
stale

   
testing

   
wallet

   
weekly-report

   
wontfix

This will not be worked on

No labels
DB & Storage
Deployment
Error Handling & Logging
Maintenance
Payment Backend
backport
backport v0.13.x
backport v0.14.x
backport v0.15.x
bindings
blocked
bug
cdk-sql
ci
cli
deps
documentation
duplicate
enhancement
good first issue
help wanted
invalid
keep-open
ldk-node-ui
migrations
mint
mutation-testing
needs rebase
needs review
new nut
nut change
question
ready
rust-version
rustfmt
stacked hold
stale
testing
wallet
weekly-report
wontfix
Milestone
Clear milestone
No items
No milestone
v0.6.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cashubtc/cdk!495
No description provided.