Add RedisOperation and RedisOperationState

[zbirenbaum]

Description

Creates groundwork for Redis State Manager implementation by introducing two new structs. RedisOperation represents an operation stored in redis and RedisOperationState represents the wrapped object which implements the ActionStateResult trait.

Type of change

• New feature (non-breaking change which adds functionality)

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[adam-singer]

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), and 2 discussions need to be resolved

---

nativelink-scheduler/BUILD.bazel line 20 at r1 (raw file):

        "src/platform_property_manager.rs",
        "src/property_modifier_scheduler.rs",
        "src/redis_operation_state.rs",

can redis stuff be scoped under a redis folder ?

---

nativelink-scheduler/src/redis_operation_state.rs line 90 at r1 (raw file):

                );
                if let Err(_e) = res {
                    todo!()

Why are these todo? Would it be odd to return a Result from a "new" constructor?

[zbirenbaum]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), and 2 discussions need to be resolved

---

nativelink-scheduler/BUILD.bazel line 20 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

can redis stuff be scoped under a redis folder ?

Let's do that in a followup if possible, happy to make a ticket for it. Right now it's all contained in one place so it's not much clutter. If we want to break it out into multiple files in a sub dir that's fine but will require a lot of overhead since there are 3 other PRs to rebase.

---

nativelink-scheduler/src/redis_operation_state.rs line 90 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Why are these todo? Would it be odd to return a Result from a "new" constructor?

Blaise and I discussed this and the error handling here get's extremely complicated. They will have to be sent like event notifications and figuring out the best way to do it will take some thought.

Returning a result from a new constructor isn't an issue, but returning it from a spawn is.

[adam-singer]

Reviewed 1 of 7 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained

[zbirenbaum]

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

---

nativelink-scheduler/src/redis_operation_state.rs line 34 at r2 (raw file):

use crate::operation_state_manager::{ActionStateResult, OperationStageFlags};

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]

I don't love this enum and I really don't like the parse_state_flags method below it. The problem is that we

---

nativelink-scheduler/src/redis_operation_state.rs line 43 at r2 (raw file):

}

fn _parse_stage_flags(flags: &OperationStageFlags) -> Vec<OperationStage> {

I added some context on why this exists in the client pr where this function is actually called. The bottom line is I really don't like it but I'm not entirely sure what the best way to do this is and am open to suggestions.

I don't think this code should be merged, it's just too verbose.

[zbirenbaum]

Changes merged in #1023