Restructure files to be more appropriate #1131

allada · 2024-07-10T06:08:48Z

Mostly just moving files and functions around to be more appropriate.

This change is

allada

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: vale (waiting on @adam-singer and @zbirenbaum)

adam-singer

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and 10 discussions need to be resolved (waiting on @zbirenbaum)

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 44 at r1 (raw file):

/// How often the owning database will have the AwaitedAction touched
/// to keep it from being evicted.
const KEEPALIVE_DURATION: Duration = Duration::from_secs(10);

Can we inline this one, rust-lang/rust#57391, lets avoid the global const for any potential weird rust bugs.

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 46 at r1 (raw file):

const KEEPALIVE_DURATION: Duration = Duration::from_secs(10);

fn filter_check(awaited_action: &AwaitedAction, filter: &OperationFilter) -> bool {

nit: is there anything else we could call this besides filter_check, filter_predicate seems to indicate what it is, tho not what the definition of the predicate is. Trying to read this and ask myself "What kind of filter predicate is this?"

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 153 at r1 (raw file):

    /// Channel to notify when a client operation id is dropped.
    client_operation_drop_tx: mpsc::UnboundedSender<Arc<AwaitedAction>>,

👍

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 174 at r1 (raw file):

                make_err!(
                    Code::Internal,
                    "Could not find action info MemorySchedulerStateManager::update_operation"

Can we make operation_id && maybe_worker_id part of this err message, that way we can possibly have clues to which operation this might of stem from? I'd bet we probably do serialize that information upstream, tho unsure.

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 183 at r1 (raw file):

            return Err(make_err!(
                Code::Internal,
                "Action {operation_id:?} is already completed with state {:?}",

Include maybe_worker_id

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 231 at r1 (raw file):

                        error: Some(err.clone().merge(make_err!(
                            Code::Internal,
                            "Job cancelled because it attempted to execute too many times and failed"

Encode the important bits of information here related to worker id, operation id, etc.

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 305 at r1 (raw file):

                    spawn!("state_manager_client_drop_rx", async move {
                        /// Number of events to pull from the stream at a time.
                        const MAX_DROP_HANDLES_PER_CYCLE: usize = 1024;

nit: we could promote this outside of the impl scope

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 276 at r1 (raw file):

                event!(
                    Level::ERROR,
                    "sorted_action_info_hash_keys and action_info_hash_key_to_awaited_action are out of sync",

nit: is there any useful serialized information we can output here that doesn't screen barf?

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 364 at r1 (raw file):

                "Could not find existing action with name: {unique_qualifier}"
            ))
            .err_tip(|| "In state_manager::try_subscribe")?;

nit: would be good to know both client_operation_id && unique_qualifier if we reach these failure cases.

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 370 at r1 (raw file):

        if awaited_action.get_current_state().stage.is_finished() {
            return Err(make_input_err!(
                "Subscribing an item that is already completed should be handled by CacheLookupScheduler."

nit: same including what the stage was

zbirenbaum

Reviewable status: 1 of 2 LGTMs obtained, and 11 discussions need to be resolved

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/client_awaited_action.rs line 70 at r1 (raw file):

/// Trait to be able to use the [`EvictingMap`] with [`ClientAwaitedAction`].
/// Note: We only use [`EvictingMap`] for a time based evictiong, which is

It doesn't seem like ClientAwaitedAction is iterable so having values for these at all seems a bit strange

allada

Reviewable status: 1 of 2 LGTMs obtained, and 5 discussions need to be resolved (waiting on @zbirenbaum)

nativelink-scheduler/src/memory_scheduler_state/memory_scheduler_state_manager.rs line 44 at r1 (raw file):