Message field can now be populated with useful messages #361

allada · 2023-10-31T06:35:32Z

We now allow users to configure how the message field is populated for ActionResult and added HistoricalExecuteResponse that is compatible with bb_browswer.

closes #333
closes #331

This change is

allada

+@aaronmondal
cc: @chrisstaite-menlo (feel free to review too if you'd like)

Reviewable status: 0 of 20 files reviewed, all discussions resolved (waiting on @aaronmondal)

aaronmondal

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @allada)

cas/cas_main.rs line 501 at r1 (raw file):

                    let maybe_ac_store = if let Some(ac_store_ref) = &local_worker_cfg.upload_action_result.ac_store {
                        Some(store_manager.get_store(ac_store_ref).err_tip(|| {
                            format!("Failed to find store for ac_store in worker config : {}", ac_store_ref)

nit: ac_store_ref can be inlined:format!("Failed to find ... {ac_store_ref}")

cas/cas_main.rs line 512 at r1 (raw file):

                            store_manager.get_store(cas_store_ref).err_tip(|| {
                                format!(
                                    "Failed to find store for historical_results_store in worker config : {}",

nit: cas_store_ref can be inlined:format!("Failed to find ... {cas_store_ref}")

cas/worker/running_actions_manager.rs line 1181 at r1 (raw file):

        if treat_infra_error_as_failure && action_result.error.is_some() {
            did_fail = true;
        }

Nit: You could remove the mut here

Suggestion:

        let did_fail = if treat_infra_error_as_failure && action_result.error.is_some() {
            true
        } else {
            action_result.exit_code != 0
        };

cas/worker/running_actions_manager.rs line 1187 at r1 (raw file):

            // Never cache internal errors or timeouts.
            UploadCacheResultsStrategy::Everything => treat_infra_error_as_failure || action_result.error.is_none(),
            UploadCacheResultsStrategy::FailuresOnly => did_fail,

BTW Interesting. What is the use case for the FailuresOnly strategy?

cas/worker/running_actions_manager.rs line 1199 at r1 (raw file):

    ) -> Result<String, Error> {
        // TODO(allada) Currently only sha256 is supported, but soon will be dynamic.
        template_str.replace("digest_function", digest_function::Value::Sha256.as_str_name());

Nit Do we really want to use SHA256 as the default over Blake3?

cas/worker/running_actions_manager.rs line 1235 at r1 (raw file):

                .update_action_result(Request::new(update_action_request))
                .await
                .map(|_| ())

Is this safe to do? What are we mapping to () here?

cas/worker/running_actions_manager.rs line 1285 at r1 (raw file):

        let mut execute_response = to_execute_response(action_result.clone());

        let message_template = if action_result.exit_code == 0 && action_result.error.is_none() {

Could you add a comment explaining in what situation we could we get an exit code 0 and a not-none error message?

cas/worker/running_actions_manager.rs line 1325 at r1 (raw file):

            )
            .await
            .map(|_| ())

What are we mapping here?

allada

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronmondal)

cas/cas_main.rs line 501 at r1 (raw file):