Skip to content

Commit

Permalink
Fix clippy warnings for scheduler directory
Browse files Browse the repository at this point in the history
  • Loading branch information
aaronmondal committed Jul 14, 2023
1 parent f118ccd commit 1491d0a
Show file tree
Hide file tree
Showing 7 changed files with 257 additions and 254 deletions.
36 changes: 7 additions & 29 deletions cas/grpc_service/tests/worker_api_server_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,7 @@ pub mod connect_worker_tests {
pub async fn connect_worker_adds_worker_to_scheduler_test() -> Result<(), Box<dyn std::error::Error>> {
let test_context = setup_api_server(BASE_WORKER_TIMEOUT_S, Box::new(static_now_fn)).await?;

let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(worker_exists, "Expected worker to exist in worker map");

Ok(())
Expand All @@ -132,20 +129,14 @@ pub mod keep_alive_tests {
// Now change time to 1 second before timeout and ensure the worker is still in the pool.
now_timestamp += BASE_WORKER_TIMEOUT_S - 1;
test_context.scheduler.remove_timedout_workers(now_timestamp).await?;
let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(worker_exists, "Expected worker to exist in worker map");
}
{
// Now add 1 second and our worker should have been evicted due to timeout.
now_timestamp += 1;
test_context.scheduler.remove_timedout_workers(now_timestamp).await?;
let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(!worker_exists, "Expected worker to not exist in map");
}

Expand All @@ -171,10 +162,7 @@ pub mod keep_alive_tests {
// Now change time to 1 second before timeout and ensure the worker is still in the pool.
let timestamp = add_and_return_timestamp(BASE_WORKER_TIMEOUT_S - 1);
test_context.scheduler.remove_timedout_workers(timestamp).await?;
let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(worker_exists, "Expected worker to exist in worker map");
}
{
Expand All @@ -191,10 +179,7 @@ pub mod keep_alive_tests {
// Now add 1 second and our worker should still exist in our map.
let timestamp = add_and_return_timestamp(1);
test_context.scheduler.remove_timedout_workers(timestamp).await?;
let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(worker_exists, "Expected worker to exist in map");
}

Expand All @@ -209,7 +194,6 @@ pub mod keep_alive_tests {
test_context
.scheduler
.send_keep_alive_to_worker_for_test(&test_context.worker_id)
.await
.err_tip(|| "Could not send keep alive to worker")?;

{
Expand Down Expand Up @@ -240,18 +224,12 @@ pub mod going_away_tests {
pub async fn going_away_removes_worker_test() -> Result<(), Box<dyn std::error::Error>> {
let test_context = setup_api_server(BASE_WORKER_TIMEOUT_S, Box::new(static_now_fn)).await?;

let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(worker_exists, "Expected worker to exist in worker map");

test_context.scheduler.remove_worker(test_context.worker_id).await;

let worker_exists = test_context
.scheduler
.contains_worker_for_test(&test_context.worker_id)
.await;
let worker_exists = test_context.scheduler.contains_worker_for_test(&test_context.worker_id);
assert!(!worker_exists, "Expected worker to be removed from worker map");

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion cas/scheduler/default_scheduler_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub fn scheduler_factory<'a>(
Box::pin(async move {
let scheduler: SchedulerFactoryResults = match scheduler_type_cfg {
SchedulerConfig::simple(config) => {
let scheduler = Arc::new(SimpleScheduler::new(&config));
let scheduler = Arc::new(SimpleScheduler::new(config));
(Some(scheduler.clone()), Some(scheduler))
}
SchedulerConfig::grpc(config) => (Some(Arc::new(GrpcScheduler::new(config).await?)), None),
Expand Down
1 change: 0 additions & 1 deletion cas/scheduler/grpc_scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use tonic::{transport, Request};

use action_messages::{ActionInfo, ActionState, DEFAULT_EXECUTION_PRIORITY};
use common::log;
use config;
use error::{make_err, Code, Error, ResultExt};
use platform_property_manager::PlatformPropertyManager;
use proto::build::bazel::remote::execution::v2::{
Expand Down
43 changes: 23 additions & 20 deletions cas/scheduler/platform_property_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,29 +31,31 @@ pub struct PlatformProperties {
}

impl PlatformProperties {
pub fn new(map: HashMap<String, PlatformPropertyValue>) -> Self {
#[must_use]
pub const fn new(map: HashMap<String, PlatformPropertyValue>) -> Self {
Self { properties: map }
}

/// Determines if the worker's `PlatformProperties` is satisfied by this struct.
pub fn is_satisfied_by(&self, worker_properties: &PlatformProperties) -> bool {
#[must_use]
pub fn is_satisfied_by(&self, worker_properties: &Self) -> bool {
for (property, check_value) in &self.properties {
if let Some(worker_value) = worker_properties.properties.get(property) {
if !check_value.is_satisfied_by(&worker_value) {
if !check_value.is_satisfied_by(worker_value) {
return false;
}
} else {
return false;
}
}
return true;
true
}
}

impl From<ProtoPlatform> for PlatformProperties {
fn from(platform: ProtoPlatform) -> Self {
let mut properties = HashMap::with_capacity(platform.properties.len());
for property in platform.properties.into_iter() {
for property in platform.properties {
properties.insert(property.name, PlatformPropertyValue::Unknown(property.value));
}
Self { properties }
Expand Down Expand Up @@ -82,41 +84,42 @@ pub enum PlatformPropertyValue {

impl PlatformPropertyValue {
/// Same as `PlatformProperties::is_satisfied_by`, but on an individual value.
pub fn is_satisfied_by(&self, worker_value: &PlatformPropertyValue) -> bool {
#[must_use]
pub fn is_satisfied_by(&self, worker_value: &Self) -> bool {
if self == worker_value {
return true;
}
match self {
PlatformPropertyValue::Minimum(v) => {
if let PlatformPropertyValue::Minimum(worker_v) = worker_value {
Self::Minimum(v) => {
if let Self::Minimum(worker_v) = worker_value {
return worker_v >= v;
}
false
}
// Priority is used to pass info to the worker and not restrict which
// workers can be selected, but might be used to prefer certain workers
// over others.
PlatformPropertyValue::Priority(_) => true,
Self::Priority(_) => true,
// Success exact case is handled above.
PlatformPropertyValue::Exact(_) => false,
// Used mostly for transporting data. This should not be relied upon when this value.
PlatformPropertyValue::Unknown(_) => false,
Self::Exact(_) | Self::Unknown(_) => false,
}
}
}

/// Helps manage known properties and conversion into PlatformPropertyValue.
/// Helps manage known properties and conversion into `PlatformPropertyValue`.
pub struct PlatformPropertyManager {
known_properties: HashMap<String, PropertyType>,
}

impl PlatformPropertyManager {
pub fn new(known_properties: HashMap<String, PropertyType>) -> Self {
#[must_use]
pub const fn new(known_properties: HashMap<String, PropertyType>) -> Self {
Self { known_properties }
}

/// Returns the `known_properties` map.
pub fn get_known_properties(&self) -> &HashMap<String, PropertyType> {
#[must_use]
pub const fn get_known_properties(&self) -> &HashMap<String, PropertyType> {
&self.known_properties
}

Expand All @@ -126,14 +129,14 @@ impl PlatformPropertyManager {
pub fn make_prop_value(&self, key: &str, value: &str) -> Result<PlatformPropertyValue, Error> {
if let Some(prop_type) = self.known_properties.get(key) {
return match prop_type {
PropertyType::Minimum => Ok(PlatformPropertyValue::Minimum(
u64::from_str_radix(value, 10).err_tip_with_code(|e| {
PropertyType::Minimum => Ok(PlatformPropertyValue::Minimum(value.parse::<u64>().err_tip_with_code(
|e| {
(
Code::InvalidArgument,
format!("Cannot convert to platform property to u64: {} - {}", value, e),
format!("Cannot convert to platform property to u64: {value} - {e}"),
)
})?,
)),
},
)?)),
PropertyType::Exact => Ok(PlatformPropertyValue::Exact(value.to_string())),
PropertyType::Priority => Ok(PlatformPropertyValue::Priority(value.to_string())),
};
Expand Down
Loading

0 comments on commit 1491d0a

Please sign in to comment.