Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert AwaitedAction to and from raw bytes #1206

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

zbirenbaum
Copy link
Contributor

@zbirenbaum zbirenbaum commented Jul 27, 2024

Makes AwaitedAction Serialize/Deserialize and adds ability to convert to
and from raw bytes. This will be used to communicate operation data to
and external data storage such as Redis through the store API.

Towards #1182

Please delete options that aren't relevant.

  • 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 Reviewable

Copy link

cloudflare-workers-and-pages bot commented Jul 27, 2024

Deploying nativelink with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8354f09
Status: ✅  Deploy successful!
Preview URL: https://9a5a6a8c.nativelink-3e2.pages.dev
Branch Preview URL: https://awaited-action-bytes.nativelink-3e2.pages.dev

View logs

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 1 of 4 files reviewed, and 1 discussions need to be resolved


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

}

impl TryInto<Vec<u8>> for AwaitedAction {

Can we use protos or json strings instead?

the reason is because bincode is not multi-language compatible and allowing users to read the state out of redis directly (for scaling cloud instances) is a real use case.

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 LGTMs obtained, and 0 of 4 files reviewed, and pending CI: pre-commit-checks, and 1 discussions need to be resolved


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

Previously, allada (Nathan (Blaise) Bruer) wrote…

Can we use protos or json strings instead?

the reason is because bincode is not multi-language compatible and allowing users to read the state out of redis directly (for scaling cloud instances) is a real use case.

Great point, updated it to use serde_json.

As a side note I did some digging and found this really useful tool which compared the performance of various encoders/decoders and serde_json is a really poor performer in the grand scheme of things.

https://david.kolo.ski/rust_serialization_benchmark

Prost is better but would lock us out of most or possibly all redis-specific advantages for performance.

I don't think this is something to prioritize atm though, serde_json should be fine for now, and this is abstracted to just produce/consume bytes so easily updatable later.

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved


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

Previously, zbirenbaum (Zach Birenbaum) wrote…

Great point, updated it to use serde_json.

As a side note I did some digging and found this really useful tool which compared the performance of various encoders/decoders and serde_json is a really poor performer in the grand scheme of things.

https://david.kolo.ski/rust_serialization_benchmark

Prost is better but would lock us out of most or possibly all redis-specific advantages for performance.

I don't think this is something to prioritize atm though, serde_json should be fine for now, and this is abstracted to just produce/consume bytes so easily updatable later.

Yeah, performance is not something that is critical here, since we are going to be able to paralyze this. We could also make this a config option at some point.

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @allada from a discussion.
Reviewable status: 1 of 1 LGTMs obtained, and 2 of 4 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Cloudflare Pages, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable


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

Previously, allada (Nathan (Blaise) Bruer) wrote…

Yeah, performance is not something that is critical here, since we are going to be able to paralyze this. We could also make this a config option at some point.

Sounds good

@zbirenbaum zbirenbaum enabled auto-merge (squash) July 29, 2024 01:47
@zbirenbaum zbirenbaum disabled auto-merge July 29, 2024 17:32
Makes AwaitedAction Serialize/Deserialize and adds ability to convert to
and from raw bytes. This will be used to communicate operation data to
and external data storage such as Redis through the store API.
Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r2, 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained, and all files reviewed

@zbirenbaum zbirenbaum merged commit f004351 into main Jul 29, 2024
30 checks passed
@zbirenbaum zbirenbaum deleted the awaited-action-bytes branch July 29, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants