From 93ee5ca13d76230e6b761d41fbeaff96b6ae97d6 Mon Sep 17 00:00:00 2001 From: Pierre Chevalier Date: Wed, 7 Aug 2019 12:21:19 +0100 Subject: [PATCH] TimeSpan: use result to express possible failure Let the caller decide how to deal with the error case (by warning) --- src/rust/engine/Cargo.lock | 1 - src/rust/engine/concrete_time/Cargo.toml | 2 - src/rust/engine/concrete_time/src/lib.rs | 21 ++--- .../engine/process_execution/src/remote.rs | 86 +++++++++++-------- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 119ed58d94a7..80cffbf6b31b 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -381,7 +381,6 @@ dependencies = [ name = "concrete_time" version = "0.0.1" dependencies = [ - "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "protobuf 2.0.6 (git+https://github.com/pantsbuild/rust-protobuf?rev=171611c33ec92f07e1b7107327f6d0139a7afebf)", "serde 1.0.98 (registry+https://github.com/rust-lang/crates.io-index)", "serde_derive 1.0.98 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/src/rust/engine/concrete_time/Cargo.toml b/src/rust/engine/concrete_time/Cargo.toml index 8bdddbf9dbf6..1772d06aa89a 100644 --- a/src/rust/engine/concrete_time/Cargo.toml +++ b/src/rust/engine/concrete_time/Cargo.toml @@ -9,5 +9,3 @@ publish = false protobuf = { version = "2.0.6", features = ["with-bytes"] } serde_derive = "1.0.98" serde = "1.0.98" -log = "0.4.8" - diff --git a/src/rust/engine/concrete_time/src/lib.rs b/src/rust/engine/concrete_time/src/lib.rs index 94ff023ba8e2..e67ceb580733 100644 --- a/src/rust/engine/concrete_time/src/lib.rs +++ b/src/rust/engine/concrete_time/src/lib.rs @@ -26,7 +26,6 @@ // Arc can be more clear than needing to grok Orderings: #![allow(clippy::mutex_atomic)] -use log::warn; use serde_derive::Serialize; /// A concrete data representation of a duration. @@ -107,20 +106,19 @@ impl TimeSpan { start: &protobuf::well_known_types::Timestamp, end: &protobuf::well_known_types::Timestamp, time_span_description: &str, - ) -> Option { + ) -> Result { let start = Self::std_duration_from_protobuf_timestamp(start); let end = Self::std_duration_from_protobuf_timestamp(end); let time_span = end.checked_sub(start).map(|duration| TimeSpan { start: start.into(), duration: duration.into(), }); - if time_span.is_none() { - warn!( + time_span.ok_or_else(|| { + format!( "Got negative {} time: {:?} - {:?}", time_span_description, end, start - ); - } - time_span + ) + }) } } @@ -158,7 +156,10 @@ mod tests { ); } - fn time_span_from_start_and_duration_in_seconds(start: i64, duration: i64) -> Option { + fn time_span_from_start_and_duration_in_seconds( + start: i64, + duration: i64, + ) -> Result { use protobuf::well_known_types::Timestamp; let mut start_timestamp = Timestamp::new(); start_timestamp.set_seconds(start); @@ -171,7 +172,7 @@ mod tests { fn time_span_from_start_and_end_given_positive_duration() { let span = time_span_from_start_and_duration_in_seconds(42, 10); assert_eq!( - Some(TimeSpan { + Ok(TimeSpan { start: Duration::new(42, 0), duration: Duration::new(10, 0), }), @@ -182,6 +183,6 @@ mod tests { #[test] fn time_span_from_start_and_end_given_negative_duration() { let span = time_span_from_start_and_duration_in_seconds(42, -10); - assert!(span.is_none()); + assert!(span.is_err()); } } diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs index 1e0e29e536c5..a4ea2f0442bf 100644 --- a/src/rust/engine/process_execution/src/remote.rs +++ b/src/rust/engine/process_execution/src/remote.rs @@ -13,7 +13,7 @@ use fs::{self, File, PathStat}; use futures::{future, Future, Stream}; use grpcio; use hashing::{Digest, Fingerprint}; -use log::{debug, trace}; +use log::{debug, trace, warn}; use protobuf::{self, Message, ProtobufEnum}; use sha2::Sha256; use store::{Snapshot, Store, StoreFileByDigest}; @@ -411,64 +411,76 @@ impl CommandRunner { let parent_id = get_parent_id(); let result_cached = execute_response.get_cached_result(); - if let Some(time_span) = TimeSpan::from_start_and_end( + match TimeSpan::from_start_and_end( metadata.get_queued_timestamp(), metadata.get_worker_start_timestamp(), "remote queue", ) { - attempts.current_attempt.remote_queue = Some(time_span.duration.into()); - maybe_add_workunit( - result_cached, - "remote execution action scheduling", - time_span, - parent_id.clone(), - &workunit_store, - ); + Ok(time_span) => { + attempts.current_attempt.remote_queue = Some(time_span.duration.into()); + maybe_add_workunit( + result_cached, + "remote execution action scheduling", + time_span, + parent_id.clone(), + &workunit_store, + ); + } + Err(s) => warn!("{}", s), }; - if let Some(time_span) = TimeSpan::from_start_and_end( + match TimeSpan::from_start_and_end( metadata.get_input_fetch_start_timestamp(), metadata.get_input_fetch_completed_timestamp(), "remote input fetch", ) { - attempts.current_attempt.remote_input_fetch = Some(time_span.duration.into()); - maybe_add_workunit( - result_cached, - "remote execution worker input fetching", - time_span, - parent_id.clone(), - &workunit_store, - ); + Ok(time_span) => { + attempts.current_attempt.remote_input_fetch = Some(time_span.duration.into()); + maybe_add_workunit( + result_cached, + "remote execution worker input fetching", + time_span, + parent_id.clone(), + &workunit_store, + ); + } + Err(s) => warn!("{}", s), } - if let Some(time_span) = TimeSpan::from_start_and_end( + match TimeSpan::from_start_and_end( metadata.get_execution_start_timestamp(), metadata.get_execution_completed_timestamp(), "remote execution", ) { - attempts.current_attempt.remote_execution = Some(time_span.duration.into()); - maybe_add_workunit( - result_cached, - "remote execution worker command executing", - time_span, - parent_id.clone(), - &workunit_store, - ); + Ok(time_span) => { + attempts.current_attempt.remote_execution = Some(time_span.duration.into()); + maybe_add_workunit( + result_cached, + "remote execution worker command executing", + time_span, + parent_id.clone(), + &workunit_store, + ); + } + Err(s) => warn!("{}", s), } - if let Some(time_span) = TimeSpan::from_start_and_end( + match TimeSpan::from_start_and_end( metadata.get_output_upload_start_timestamp(), metadata.get_output_upload_completed_timestamp(), "remote output store", ) { - attempts.current_attempt.remote_output_store = Some(time_span.duration.into()); - maybe_add_workunit( - result_cached, - "remote execution worker output uploading", - time_span, - parent_id, - &workunit_store, - ); + Ok(time_span) => { + attempts.current_attempt.remote_output_store = Some(time_span.duration.into()); + maybe_add_workunit( + result_cached, + "remote execution worker output uploading", + time_span, + parent_id, + &workunit_store, + ); + } + Err(s) => warn!("{}", s), } attempts.current_attempt.was_cache_hit = execute_response.cached_result; }