TimeSpan: use result to express possible failure

Let the caller decide how to deal with the error case (by warning)

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"
-

src/rust/engine/concrete_time/src/lib.rs

@@ -26,7 +26,6 @@
 // Arc<Mutex> 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<Self> {
+  ) -> Result<Self, String> {
     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<TimeSpan> {
+  fn time_span_from_start_and_duration_in_seconds(
+    start: i64,
+    duration: i64,
+  ) -> Result<TimeSpan, String> {
     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());
   }
 }

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;
         }