Skip to content
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.

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

feat(routing): Use Moka cache for routing with cache invalidation #3216

Merged
merged 18 commits into from
May 22, 2024

Conversation

Sarthak1799
Copy link
Contributor

@Sarthak1799 Sarthak1799 commented Dec 28, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Enabled Moka cache for routing with cache invalidation

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Look for the pub-sub cache invalidation log
image
image

  1. Update an existing Merchant connector account and look for Cgrpah cache invalidation log
curl --location --request POST 'http://localhost:8080/account/sarthak/connectors/mca_VaZpO5COiFDUIiMq7DFV' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--header 'api-key: test_admin' \
--data-raw '{
    "connector_type": "fiz_operations",
    "metadata": {
        "city": "NY",
        "unit": "248"
    }
}'
  1. Activate a routing config from dashboard, then search for routing cache invalidation using routing config key

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@Sarthak1799 Sarthak1799 added C-feature Category: Feature request or enhancement A-routing Area: Routing labels Dec 28, 2023
@Sarthak1799 Sarthak1799 self-assigned this Dec 28, 2023
@Sarthak1799 Sarthak1799 requested review from a team as code owners December 28, 2023 12:48
@Sarthak1799 Sarthak1799 linked an issue Dec 28, 2023 that may be closed by this pull request
2 tasks
crates/router/src/core/admin.rs Outdated Show resolved Hide resolved
crates/router/src/core/routing/helpers.rs Outdated Show resolved Hide resolved
crates/router/src/db/merchant_account.rs Outdated Show resolved Hide resolved
Comment on lines 475 to 480
[
cache::CacheKind::Accounts(
format!("{}_{}", mca.merchant_id, _profile_id).into(),
),
cache::CacheKind::KGraph(
format!("kgraph_{}_{_profile_id}", mca.merchant_id).into(),
Copy link
Member

Choose a reason for hiding this comment

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

Feature gating for business_profile_routing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a check for profile_id here, we seem to be throwing an error in case profile_id is not present.

@Sarthak1799 Sarthak1799 added the R-waiting-on-L1 Review: Waiting on L1 reviewer label Jan 8, 2024
crates/storage_impl/src/redis/cache.rs Outdated Show resolved Hide resolved
dracarys18
dracarys18 previously approved these changes Jan 8, 2024
crates/router/src/core/payments/routing.rs Outdated Show resolved Hide resolved
crates/router/src/db/merchant_account.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments/routing.rs Show resolved Hide resolved
Comment on lines 408 to 420
let kgraph_key = merchant_account.default_profile.as_ref().map(|profile_id| {
CacheKind::KGraph(
format!(
"kgraph_{}_{}",
merchant_account.merchant_id.clone(),
profile_id,
)
.into(),
)
});

#[cfg(not(feature = "business_profile_routing"))]
let kgraph_key = Some(format!("kgraph_{}", merchant_account.merchant_id.clone()));
Copy link
Member

Choose a reason for hiding this comment

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

The types for both variables do not match. Ensure that they do.

vspecky
vspecky previously approved these changes May 21, 2024
Copy link
Member

@vspecky vspecky left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks. Not blockers.

crates/router/src/core/payments/routing.rs Outdated Show resolved Hide resolved
crates/storage_impl/src/redis/cache.rs Outdated Show resolved Hide resolved
crates/storage_impl/src/redis/cache.rs Outdated Show resolved Hide resolved
crates/storage_impl/src/redis/cache.rs Outdated Show resolved Hide resolved
vspecky
vspecky previously approved these changes May 21, 2024
dracarys18
dracarys18 previously approved these changes May 21, 2024
SanchithHegde
SanchithHegde previously approved these changes May 21, 2024
@likhinbopanna likhinbopanna added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 21, 2024
@Sarthak1799 Sarthak1799 dismissed stale reviews from SanchithHegde, dracarys18, and vspecky via 47ca2d4 May 22, 2024 07:43
@likhinbopanna likhinbopanna enabled auto-merge May 22, 2024 08:10
@likhinbopanna likhinbopanna added this pull request to the merge queue May 22, 2024
Merged via the queue into main with commit 431560b May 22, 2024
10 checks passed
@likhinbopanna likhinbopanna deleted the routing-cache-moka branch May 22, 2024 08:46
@SanchithHegde SanchithHegde removed the R-waiting-on-L1 Review: Waiting on L1 reviewer label May 26, 2024
Comment on lines +517 to +526
let kgraph_key = merchant_account.default_profile.as_ref().map(|profile_id| {
CacheKind::CGraph(
format!(
"kgraph_{}_{}",
merchant_account.merchant_id.clone(),
profile_id,
)
.into(),
)
});
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been cgraph_key, and the format string "cgraph_{}_{}"?

Comment on lines +529 to +531
let kgraph_key = Some(CacheKind::CGraph(
format!("kgraph_{}", merchant_account.merchant_id.clone()).into(),
));
Copy link
Member

Choose a reason for hiding this comment

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

Same here, should the variable name have been cgraph_key and the format string cgraph_{}?

Why don't we consider adding a function to generate the cgraph cache key for a specified merchant ID, and an optionally specified business profile ID (the profile ID could also be specified based on the business_profile_routing being enabled)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-routing Area: Routing C-feature Category: Feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Replace Routing cache with Moka
5 participants