-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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): Contract based routing integration #6761
base: main
Are you sure you want to change the base?
Conversation
…r-router-integration
fn get_enabled_features(&mut self) -> &mut DynamicRoutingFeatures { | ||
&mut self.enabled_feature | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is supposed to be a get call right as per the name, but we are returning mutable reference. should we have separate method for get and set instead of returning mutable reference.
Can be taken in separate PR
if let Some(contract_based_routing) = new.contract_based_routing { | ||
self.contract_based_routing = Some(contract_based_routing) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this update
method currently updates all the fields. its better to have separate update methods for each fields to improve visibility
Again can be taken up in separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is updating the algorithm right?
…r-router-integration
.transpose() | ||
.change_context(errors::ApiErrorResponse::InternalServerError) | ||
.attach_printable("unable to deserialize DynamicRoutingAlgorithmRef from JSON")? | ||
.unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just do if let on business_profile.dynamic_routing_algorithm
instead of doing unwrap_or_default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let expressions in the same condition are unstable. We'd have to have two nested loops then.
I'll try to find a way better way of doing this
connector_list = dynamic_routing_algo_ref | ||
.contract_based_routing | ||
.as_ref() | ||
.async_map(|algorithm| { | ||
perform_contract_based_routing( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also do we want to go forward with performing contract based routing even if success based routing failed? because I see that you are only logging the error after success based failed and doing unwrap_or to original connector list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Success routing can only fail here if,
It's set up but call fails, in this case contract won't be activated so the call won't happen.
It's not set up, in which case call won't happen and contract call would take place next.
crates/router/src/core/routing.rs
Outdated
for info in info_vec { | ||
let mca = db | ||
.find_by_merchant_connector_account_merchant_id_merchant_connector_id( | ||
key_manager_state, | ||
merchant_account.get_id(), | ||
&info.mca_id, | ||
&key_store, | ||
) | ||
.await | ||
.change_context(errors::ApiErrorResponse::MerchantConnectorAccountNotFound { | ||
id: info.mca_id.get_string_repr().to_owned(), | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do tokio join?
@@ -1419,6 +1463,292 @@ pub async fn success_based_routing_update_configs( | |||
Ok(service_api::ApplicationResponse::Json(new_record)) | |||
} | |||
|
|||
#[cfg(all(feature = "v1", feature = "dynamic_routing"))] | |||
pub async fn contract_based_dynamic_routing_setup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why didn't you make use of existing toggle_specific_dynamic_routing
function to activate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because CR cannot be set to default, we can use that for routing types that can be always be toggled to some default config.
first_contract_based_connector.to_string(), | ||
); | ||
|
||
core_metrics::DYNAMIC_CONTRACT_BASED_ROUTING.add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this metric be pushed only if payment_status_attribute
is charged
?
state: &SessionState, | ||
routable_connectors: Vec<api_routing::RoutableConnectorChoice>, | ||
profile_id: &common_utils::id_type::ProfileId, | ||
_dynamic_routing_config_params_interpolator: routing::helpers::DynamicRoutingConfigParamsInterpolator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this, if redundant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had kept this as we had decided to use the params in the upcoming PRs,
…r-router-integration
4f12340
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Response -
Metrics Population post payment -
Checklist
cargo +nightly fmt --all
cargo clippy