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: Soft kill kv #4582

Merged
merged 21 commits into from
May 20, 2024
Merged

feat: Soft kill kv #4582

merged 21 commits into from
May 20, 2024

Conversation

akshay-97
Copy link
Contributor

@akshay-97 akshay-97 commented May 8, 2024

Type of Change

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

Description

We needed a way to gracefully disable kv with ongoing traffic in our services, Introducing soft kill mode(env switch) that when enabled will direct new inserts to db and updates of data present in kv to go to kv and other wise db

Additional Changes

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

Motivation and Context

We needed a way to gracefully disable kv with ongoing traffic in our services, Introducing soft kill mode(env switch) that when enabled will direct new inserts to db and updates of data present in kv to go to kv and other wise db

https://docs.google.com/document/d/1AvIID-rPRpQ7xy_VgUAeDWSkG0tdbY3xiz5OzBopTdw

How did you test it?

Do transactions in kv mode, run app in soft kill mode and check if existing payments in kv go through kv and new ones are in db

when in soft kill, storage scheme decided output log
[decide_storage_scheme] output: postgres_only for entity diesel_models::payment_intent::PaymentIntent

during soft kill mode, no merchant can be enabled for kv flow
{ "error": { "type": "invalid_request", "message": "Kv cannot be enabled when application is in soft_kill_mode", "code": "IR_06" } }

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

@akshay-97 akshay-97 requested review from a team as code owners May 8, 2024 08:28
@akshay-97 akshay-97 self-assigned this May 13, 2024
crates/router/src/db/address.rs Outdated Show resolved Hide resolved
Comment on lines 212 to 215
let key = PartitionKey::MerchantIdCustomerId {
merchant_id: merchant_id.as_str(),
customer_id: customer_id.as_str(),
};
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 redundant partition key

crates/router/src/db/mandate.rs Outdated Show resolved Hide resolved
@@ -143,6 +143,9 @@ impl RedisErrorExt for error_stack::Report<RedisError> {
key: Some(key.to_string()),
})
}
RedisError::KvSoftKillMode => {
self.change_context(DataStorageError::RedisOperationDisabled)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we converting into this error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove this, was added for another approach

crates/storage_impl/src/redis/kv_store.rs Outdated Show resolved Hide resolved
@akshay-97 akshay-97 added the M-database-changes Metadata: This PR involves database schema changes label May 15, 2024
@akshay-97 akshay-97 changed the title Soft kill kv feat: Soft kill kv May 15, 2024
dracarys18
dracarys18 previously approved these changes May 15, 2024
crates/storage_impl/src/redis/kv_store.rs Outdated Show resolved Hide resolved
crates/storage_impl/src/redis/kv_store.rs Show resolved Hide resolved
crates/storage_impl/src/redis/kv_store.rs Outdated Show resolved Hide resolved
crates/storage_impl/src/redis/kv_store.rs Show resolved Hide resolved
dracarys18
dracarys18 previously approved these changes May 16, 2024
@akshay-97 akshay-97 requested a review from jarnura May 17, 2024 11:17
inventvenkat
inventvenkat previously approved these changes May 17, 2024
@akshay-97 akshay-97 requested review from jarnura and removed request for jarnura May 17, 2024 11:22
config/development.toml Show resolved Hide resolved
crates/router/src/configs/settings.rs Show resolved Hide resolved
crates/diesel_models/src/payment_method.rs Outdated Show resolved Hide resolved
@akshay-97 akshay-97 dismissed stale reviews from inventvenkat and dracarys18 via e22d026 May 17, 2024 11:57
@Gnanasundari24 Gnanasundari24 added this pull request to the merge queue May 20, 2024
Merged via the queue into main with commit 3fa59d4 May 20, 2024
10 checks passed
@Gnanasundari24 Gnanasundari24 deleted the soft_kill_kv branch May 20, 2024 07:31
This was referenced May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-database-changes Metadata: This PR involves database schema changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants