fix: add cache to server list queries#2620
fix: add cache to server list queries#2620MasterPtato wants to merge 1 commit intographite-base/2620from
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implements caching layer for server list queries across multiple services to improve performance and reduce database load.
- Added new
server_discoverymodule inpackages/core/services/cluster/src/ops/datacenter/server_discovery.rswith 5-second TTL cache - Migrated from custom types to standard
rivet_api::modelstypes inpackages/common/service-discovery/src/lib.rs - Modified
packages/edge/infra/guard/server/src/routing/api.rsto use service discovery instead of direct server queries - Re-enabled prewarm request error handling in
packages/core/api/actor/src/route/builds.rsfor build completion safety - Added
exclude_installingflag across server list operations for better control of server lifecycle management
18 files reviewed, 6 comments
Edit PR Review Bot Settings | Greptile
| "worker" => Ok(PoolType::Worker), | ||
| "nats" => Ok(PoolType::Nats), | ||
| "guard" => Ok(PoolType::Guard), | ||
| _ => bail!("Invalid PoolType string: {}", s), |
There was a problem hiding this comment.
style: Include variant name in error message for better debugging: Invalid PoolType string '{}' (expected: job, gg, ats, etc)
| .op(crate::ops::server::list::Input { | ||
| filter: input.filter.clone(), | ||
| include_destroyed: false, | ||
| exclude_installing: false, | ||
| exclude_draining: false, | ||
| exclude_no_vlan: false, | ||
| }) |
There was a problem hiding this comment.
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.
| 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?; |
There was a problem hiding this comment.
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
| servers: servers_res | ||
| .servers | ||
| .into_iter() | ||
| // Filter out installing servers | ||
| .filter(|server| server.install_complete_ts.is_some()) | ||
| .map(ApiInto::api_into) | ||
| .collect(), |
There was a problem hiding this comment.
logic: Removal of .filter(|server| server.install_complete_ts.is_some()) could expose incomplete/installing servers to clients. Verify this is handled in server_discovery or restore the filter
| let client = Client::new(); | ||
| Ok(self.fetch_inner(&client).await?.servers) |
There was a problem hiding this comment.
style: Creating new Client for each fetch is inefficient - consider reusing the client from fetch_inner's parameter
| 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() | ||
| }; |
There was a problem hiding this comment.
style: Consider using a const for 'all' string to avoid magic strings
MasterPtato
changed the base branch from
06-16-fix_toolchain_attach_tags_to_function_workers
to
graphite-base/2620
MasterPtato
changed the base branch from
graphite-base/2620
to
06-16-fix_toolchain_attach_tags_to_function_workers
MasterPtato
changed the base branch from
06-16-fix_toolchain_attach_tags_to_function_workers
to
graphite-base/2620
MasterPtato
force-pushed
the
graphite-base/2620
branch
from
77c252b to
2125754
Compare
MasterPtato
force-pushed
the
06-16-fix_add_cache_to_server_list_queries
branch
from
5d622b5 to
0e85267
Compare
MasterPtato
changed the base branch from
graphite-base/2620
to
06-16-fix_toolchain_attach_tags_to_function_workers
NathanFlurry
changed the base branch from
06-16-fix_toolchain_attach_tags_to_function_workers
to
graphite-base/2620
Merge activity
|
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
1 participant
Changes