-
Notifications
You must be signed in to change notification settings - Fork 130
fix: add cache to server list queries #2620
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,27 +56,21 @@ pub async fn servers( | |
| _watch_index: WatchIndexQuery, | ||
| query: ServerFilterQuery, | ||
| ) -> GlobalResult<models::ProvisionDatacentersGetServersResponse> { | ||
| // Find server based on public ip | ||
| let servers_res = ctx | ||
| .op(cluster::ops::server::list::Input { | ||
| filter: cluster::types::Filter { | ||
| datacenter_ids: Some(vec![datacenter_id]), | ||
| pool_types: (!query.pools.is_empty()) | ||
| .then(|| query.pools.into_iter().map(ApiInto::api_into).collect()), | ||
| ..Default::default() | ||
| }, | ||
| include_destroyed: false, | ||
| exclude_draining: true, | ||
| exclude_no_vlan: true, | ||
| .op(cluster::ops::datacenter::server_discovery::Input { | ||
| datacenter_id, | ||
| pool_types: query | ||
| .pools | ||
| .into_iter() | ||
| .map(ApiInto::api_into) | ||
| .collect(), | ||
| }) | ||
| .await?; | ||
|
Comment on lines
59
to
68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Replacing server::list with server_discovery removes several critical filters (exclude_draining, exclude_no_vlan). Verify these are handled in server_discovery operation or document why they're no longer needed |
||
|
|
||
| Ok(models::ProvisionDatacentersGetServersResponse { | ||
| servers: servers_res | ||
| .servers | ||
| .into_iter() | ||
| // Filter out installing servers | ||
| .filter(|server| server.install_complete_ts.is_some()) | ||
| .map(ApiInto::api_into) | ||
| .collect(), | ||
|
Comment on lines
71
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Removal of |
||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| pub mod server_discovery; | ||
| pub mod get; | ||
| pub mod list; | ||
| pub mod location_get; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| use std::{collections::HashMap, str::FromStr}; | ||
|
|
||
| use chirp_workflow::prelude::*; | ||
|
|
||
| use crate::types::{Filter, PoolType, Server}; | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct Input { | ||
| pub datacenter_id: Uuid, | ||
| pub pool_types: Vec<PoolType>, | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct Output { | ||
| pub servers: Vec<Server>, | ||
| } | ||
|
|
||
| /// Wrapper around server::list with very short cache. | ||
| #[operation] | ||
| pub async fn cluster_datacenter_server_discovery(ctx: &OperationCtx, input: &Input) -> GlobalResult<Output> { | ||
| let cache_keys = if input.pool_types.is_empty() { | ||
| vec![(input.datacenter_id, "all".to_string())] | ||
| } else { | ||
| input | ||
| .pool_types | ||
| .iter() | ||
| .map(|pool| (input.datacenter_id, pool.to_string())) | ||
| .collect() | ||
| }; | ||
|
Comment on lines
+21
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using a const for 'all' string to avoid magic strings |
||
|
|
||
| let servers = ctx | ||
| .cache() | ||
| .ttl(5000) | ||
| .fetch_all_json("cluster.datacenter.service_discovery", cache_keys, { | ||
| let ctx = ctx.clone(); | ||
| move |mut cache, keys| { | ||
| let ctx = ctx.clone(); | ||
| async move { | ||
| let pools = keys | ||
| .into_iter() | ||
| .filter(|(_, pool)| pool != "all") | ||
| .map(|(_, pool)| PoolType::from_str(&pool)) | ||
| .collect::<GlobalResult<Vec<_>>>()?; | ||
|
|
||
| let servers_res = ctx | ||
| .op(crate::ops::server::list::Input { | ||
| filter: Filter { | ||
| datacenter_ids: Some(vec![input.datacenter_id]), | ||
| pool_types: (!pools.is_empty()).then(|| pools), | ||
| ..Default::default() | ||
| }, | ||
| include_destroyed: false, | ||
| exclude_installing: true, | ||
| exclude_draining: true, | ||
| exclude_no_vlan: true, | ||
| }) | ||
| .await?; | ||
|
|
||
| let mut servers_by_pool_type = | ||
| HashMap::with_capacity(servers_res.servers.len()); | ||
|
|
||
| for server in servers_res.servers { | ||
| servers_by_pool_type | ||
| .entry(server.pool_type) | ||
| .or_insert_with(Vec::new) | ||
| .push(server); | ||
| } | ||
|
|
||
| for (pool_type, servers) in servers_by_pool_type { | ||
| cache.resolve(&(input.datacenter_id, pool_type.to_string()), servers); | ||
| } | ||
|
|
||
| Ok(cache) | ||
| } | ||
| } | ||
| }) | ||
| .await?; | ||
|
|
||
| Ok(Output { | ||
| servers: servers.into_iter().flatten().collect(), | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ pub async fn cluster_server_taint_with_filter( | |
| .op(crate::ops::server::list::Input { | ||
| filter: input.filter.clone(), | ||
| include_destroyed: false, | ||
| exclude_installing: false, | ||
| exclude_draining: false, | ||
| exclude_no_vlan: false, | ||
| }) | ||
|
Comment on lines
21
to
27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: All filter flags are set to false, but there's no comment explaining why we want to include all server states. Consider adding a comment explaining this design decision. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| use std::{ | ||
| convert::{TryFrom, TryInto}, | ||
| net::{IpAddr, Ipv4Addr}, | ||
| str::FromStr, | ||
| }; | ||
|
|
||
| use chirp_workflow::prelude::*; | ||
|
|
@@ -107,6 +108,25 @@ impl std::fmt::Display for PoolType { | |
| } | ||
| } | ||
|
|
||
| impl FromStr for PoolType { | ||
| type Err = GlobalError; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s { | ||
| "job" => Ok(PoolType::Job), | ||
| "gg" => Ok(PoolType::Gg), | ||
| "ats" => Ok(PoolType::Ats), | ||
| "pegboard" => Ok(PoolType::Pegboard), | ||
| "pegboard-isolate" => Ok(PoolType::PegboardIsolate), | ||
| "fdb" => Ok(PoolType::Fdb), | ||
| "worker" => Ok(PoolType::Worker), | ||
| "nats" => Ok(PoolType::Nats), | ||
| "guard" => Ok(PoolType::Guard), | ||
| _ => bail!("Invalid PoolType string: {}", s), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Include variant name in error message for better debugging: |
||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, Hash)] | ||
| pub struct Hardware { | ||
| pub provider_hardware: String, | ||
|
|
@@ -144,7 +164,7 @@ impl From<rivet_config::config::rivet::BuildDeliveryMethod> for BuildDeliveryMet | |
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct Server { | ||
| pub server_id: Uuid, | ||
| pub datacenter_id: Uuid, | ||
|
|
||
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.
style: Creating new Client for each fetch is inefficient - consider reusing the client from fetch_inner's parameter