-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(euclid): add dynamic routing in core flows #6333
base: main
Are you sure you want to change the base?
Conversation
Review changes with SemanticDiff. Analyzed 11 of 11 files. Overall, the semantic diff is 26% smaller than the GitHub diff.
|
self.success_based_algorithm = Some(SuccessBasedAlgorithm { | ||
algorithm_id_with_timestamp: DynamicAlgorithmWithTimestamp { | ||
algorithm_id: Some(new_id), | ||
timestamp: common_utils::date_time::now_unix_timestamp(), | ||
}, | ||
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.
Nit:
Can use constructor
crates/router/src/core/routing.rs
Outdated
let _ = cache::publish_into_redact_channel( | ||
state.store.get_cache_store().as_ref(), | ||
cache_entries_to_redact, | ||
) | ||
.await | ||
.to_not_found_response(errors::ApiErrorResponse::ResourceIdNotFound)?; | ||
let response = record.foreign_into(); | ||
helpers::update_business_profile_active_dynamic_algorithm_ref( | ||
db, | ||
key_manager_state, | ||
&key_store, | ||
business_profile, | ||
dynamic_routing_algorithm, | ||
) | ||
.await?; | ||
|
||
metrics::ROUTING_UNLINK_CONFIG_SUCCESS_RESPONSE.add( | ||
&metrics::CONTEXT, | ||
1, | ||
&add_attributes([("profile_id", profile_id.get_string_repr().to_owned())]), | ||
); | ||
|
||
Ok(service_api::ApplicationResponse::Json(response)) | ||
} else { | ||
Err(errors::ApiErrorResponse::PreconditionFailed { | ||
message: "Algorithm is already inactive".to_string(), | ||
})? | ||
.map_err(|e| { | ||
logger::error!( | ||
"unable to publish into the redact channel for evicting the success based routing config cache {e:?}" | ||
) | ||
}); |
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.
Shouldn't we propagate the error if publishing to cache fails? because core will still get the configs from cache and may perform SR?
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.
Yes, propagation seems fine here.
Please add debug logs wherever necessary |
crates/router/src/core/payments.rs
Outdated
@@ -4579,40 +4579,35 @@ where | |||
.await | |||
.change_context(errors::ApiErrorResponse::InternalServerError)?; | |||
|
|||
let connectors = routing::perform_eligibility_analysis_with_fallback( |
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.
I see this is adding the fallback as well to the list and this list is getting sent to perform SR. We should only perform eligibility analysis on the connectors and send only these filtered connectors to perform SR instead of adding the fallback too. Fallback connectors should be appended to the list obtained after performing SR
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 will be addressed in this PR.
use crate::{core::metrics as core_metrics, routes::metrics}; | ||
#[cfg(feature = "v1")] | ||
use crate::{core::metrics as core_metrics, routes::metrics, types::transformers::ForeignInto}; | ||
pub const DYNAMIC_ALGORITHM_NAME: &str = "Dynamic routing algorithm"; |
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.
Isn't this a success routing algorithm?
Type of Change
Description
This will add dynamic_routing in core flows.
After which once the toggle route is hit with
feature = dynamic_connector_selection
, routable connectors will be evaluated on basis of success_based routing.Additional Changes
Motivation and Context
Will enable success_based routing
How did you test it?
The testing flows are as follows:
dynamic_connector_selection
.Checklist
cargo +nightly fmt --all
cargo clippy