Migrate to hyper 1.x, axum 0.7.x, tonic 0.12.x

[aaronmondal]

https://www.youtube.com/watch?v=7Twnmhe948A&ab_channel=Kontor.TV

---

This change is 

[aaronmondal]

+@adam-singer +@allada +@caass +@jhpratt +@zbirenbaum

Only the second commit here is relevant. The other one is #1159

Reviewable status: 0 of 5 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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 (waiting on @adam-singer, @allada, @caass, @jhpratt, and @zbirenbaum)

[allada]

Reviewed 12 of 34 files at r2.
Reviewable status: 0 of 5 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 18 discussions need to be resolved (waiting on @adam-singer, @caass, @jhpratt, and @zbirenbaum)

---

nativelink-service/tests/bytestream_server_test.rs line 93 at r2 (raw file):

    encoding: Option<CompressionEncoding>,
) -> (
    tokio::sync::mpsc::Sender<Frame<Bytes>>,

nit: shorten this to mpsc::Sender... This is how most other places in the code do it.

---

nativelink-service/tests/bytestream_server_test.rs line 107 at r2 (raw file):

    bs_server: Arc<ByteStreamServer>,
    encoding: Option<CompressionEncoding>,
) -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, JoinHandle) {

ditto.

---

nativelink-service/tests/bytestream_server_test.rs line 152 at r2 (raw file):

                let body = body
                    .map_err(|e| tonic::Status::internal(e.to_string()))
                    .boxed_unsync();

I wounder if we can do this without a box?

---

nativelink-util/Cargo.toml line 17 at r2 (raw file):

# TODO(aaronmondal): This is the commit that migrates tonic 0.11 to 0.12. Use a
#                    regular version once console-subscriber 0.4.0 is released.
console-subscriber = { git = "https://github.com/tokio-rs/console", rev = "5f6faa2" }

nit: Shouldn't this be under a feature?

---

nativelink-util/src/channel_body.rs line 1 at r2 (raw file):

// Copyright 2023 The NativeLink Authors. All rights reserved.

nit: Should we rename this channel_body_for_test (and rename the struct)? This should reduce the chances of it being used in production code.

---

nativelink-util/src/channel_body.rs line 26 at r2 (raw file):

    pub struct ChannelBody {
        #[pin]
        rx: tokio::sync::mpsc::Receiver<Frame<Bytes>>,

ditto.

---

nativelink-util/src/channel_body.rs line 35 at r2 (raw file):

impl ChannelBody {
    #[must_use]
    pub fn new() -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, Self) {

ditto.

---

nativelink-util/src/channel_body.rs line 36 at r2 (raw file):

    #[must_use]
    pub fn new() -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, Self) {
        Self::with_buffer_size(32)

nit: Magic numbers should be a const and at top of file.

---

nativelink-util/src/channel_body.rs line 40 at r2 (raw file):

    #[must_use]
    pub fn with_buffer_size(buffer_size: usize) -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, Self) {

ditto.

---

nativelink-util/src/channel_body.rs line 41 at r2 (raw file):

    #[must_use]
    pub fn with_buffer_size(buffer_size: usize) -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, Self) {
        let (tx, rx) = tokio::sync::mpsc::channel(buffer_size);

ditto.

---

nativelink-util/src/connection_manager.rs line 432 at r2 (raw file):

/// to allow messaging about connection success and failure.
impl tonic::codegen::Service<tonic::codegen::http::Request<tonic::body::BoxBody>> for Connection {
    type Response = tonic::codegen::http::Response<tonic::body::BoxBody>;

I'm interested to know if we can do this without a box?

---

nativelink-util/tests/channel_body_test.rs line 1 at r2 (raw file):

// Copyright 2023 The NativeLink Authors. All rights reserved.

to be honest this has way too many tests, It looks like this was taken from the tokio code? Can we just put a comment in here pointing at the file/commit this was taken from?

Second, I'd suggest just putting a couple happy path tests in. This feels like we are going to lug around these tests for something that isn't even our code.

---

nativelink-util/tests/channel_body_test.rs line 29 at r2 (raw file):

async fn setup_channel_body(
    frames: Vec<String>,
) -> (tokio::sync::mpsc::Sender<Frame<Bytes>>, ChannelBody) {

(ditto with all these in this file).

---

nativelink-util/tests/channel_body_test.rs line 59 at r2 (raw file):

generate_channel_body_tests! {
    test_channel_body_empty: [String::new()],

yuck, can we just spell these out? It makes debugging impossible when people use generated tests.

---

nativelink-util/tests/channel_body_test.rs line 115 at r2 (raw file):

    // Create a Status error from the received data
    let error = Status::internal(String::from_utf8(data.to_vec()).unwrap());

nit: This test is actually testing Status encoding/decoding, not this library. I would remove this test.

---

nativelink-util/tests/channel_body_test.rs line 171 at r2 (raw file):

#[nativelink_test]
async fn test_channel_body_capacity() {

This does not appear to be testing anything abnormal.

Yes it is sending 40 frames, but nowhere do we ensure it is receiving backpressure.

If we want this test, you can't use spawns, instead you need to poll the futures manually in step-sequence to ensure it has the intended behavior.

---

nativelink-util/tests/channel_body_test.rs line 281 at r2 (raw file):

#[nativelink_test]
async fn test_channel_body_backpressure() {

we appear to have this test above.

---

nativelink-util/tests/channel_body_test.rs line 348 at r2 (raw file):

#[nativelink_test]
async fn test_channel_body_timeout() {

remove this test, we don't allow our tests to depend on wall time.

[aaronmondal]

Reviewable status: 0 of 5 LGTMs obtained, and pending CI: pre-commit-checks, and 14 discussions need to be resolved (waiting on @adam-singer, @allada, @caass, @jhpratt, and @zbirenbaum)

---

nativelink-service/tests/bytestream_server_test.rs line 107 at r2 (raw file):

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

ditto.

Done.

---

nativelink-service/tests/bytestream_server_test.rs line 152 at r2 (raw file):

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

I wounder if we can do this without a box?

I'm not sure in this case, as it's the tonic::service::Routes that dictates it in this case. Tonic's new request body type is called BoxBody and looks like this:

type BoxBody = UnsyncBoxBody<Bytes, Status>;

https://docs.rs/tonic/latest/tonic/body/type.BoxBody.html

---

nativelink-util/Cargo.toml line 17 at r2 (raw file):

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

nit: Shouldn't this be under a feature?

I'll leave that up to @adam-singer to address in a separate PR.

---

nativelink-util/src/connection_manager.rs line 432 at r2 (raw file):

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

I'm interested to know if we can do this without a box?

AFAIU this isn't possible at the moment due to this being a direct requirement of tonics Router:

https://github.com/hyperium/tonic/blob/aa57ffebc75c8ef5caa0534d7976f43c01f86051/tonic/src/service/router.rs#L55

However, this doesn't seem to be a pessimization since apparently the previous implementation used a BoxBody internally already:

hyperium/tonic@aaede1e#diff-bf2e5f587650907b7a56b51043b1f36494faf3fb4a33e78f44cd3b99356fe207

---

nativelink-util/tests/channel_body_test.rs line 1 at r2 (raw file):

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

to be honest this has way too many tests, It looks like this was taken from the tokio code? Can we just put a comment in here pointing at the file/commit this was taken from?

Second, I'd suggest just putting a couple happy path tests in. This feels like we are going to lug around these tests for something that isn't even our code.

Done. fyi the ChannelBody is adapted from https://github.com/hyperium/tonic/pull/1670/files#diff-471242ec6300ff9b1d2be22fd95e1bd348f69eced60ec53f63e7cc5c86d2a985. I made it a bit less generic and a bit more explicit to be a hyper::body::Body.

Regarding the tests I agree. They were initially intended to showcase some examples and then I got sidetracked into making way too many.

I'll remove most of them and only keep some basic functionality for exemplary purposes. Also agreed to make this a channel_body_for_tests.

---

nativelink-util/tests/channel_body_test.rs line 29 at r2 (raw file):

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

(ditto with all these in this file).

Done.

---

nativelink-util/tests/channel_body_test.rs line 59 at r2 (raw file):

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

yuck, can we just spell these out? It makes debugging impossible when people use generated tests.

Done. My fantastic python fixtrue approach though 😆

Actually removing them altogether as we have enough fidelity from just a test_channel_body_hello test and the usage in the other tests.

---

nativelink-util/tests/channel_body_test.rs line 171 at r2 (raw file):

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

This does not appear to be testing anything abnormal.

Yes it is sending 40 frames, but nowhere do we ensure it is receiving backpressure.

If we want this test, you can't use spawns, instead you need to poll the futures manually in step-sequence to ensure it has the intended behavior.

Done. (removed). Not necessary for a test utility.

---

nativelink-util/tests/channel_body_test.rs line 281 at r2 (raw file):

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

we appear to have this test above.

Done.

---

nativelink-util/tests/channel_body_test.rs line 348 at r2 (raw file):

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

remove this test, we don't allow our tests to depend on wall time.

Done.

---

nativelink-util/src/channel_body.rs line 1 at r2 (raw file):

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

nit: Should we rename this channel_body_for_test (and rename the struct)? This should reduce the chances of it being used in production code.

Done. For the filename yes. For the struct itself I think it's a bit nicer to have it called ChannelBody. The naming convention for structs whose primary purpose is to implement the hyper::Body trait is XxxBody.

---

nativelink-util/src/channel_body.rs line 26 at r2 (raw file):

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

ditto.

Done.

---

nativelink-util/src/channel_body.rs line 35 at r2 (raw file):

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

ditto.

Done.

---

nativelink-util/src/channel_body.rs line 40 at r2 (raw file):

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

ditto.

Done.

---

nativelink-util/src/channel_body.rs line 41 at r2 (raw file):

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

ditto.

Done.

[aaronmondal]

Blocked by:

• Remove Prometheus in favor of OpenTelemetry #1164

[allada]

Reviewed 19 of 34 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: 1 of 5 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04) (waiting on @adam-singer, @caass, @jhpratt, and @zbirenbaum)

[jhpratt]

Reviewable status: 1 of 5 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), and 2 discussions need to be resolved (waiting on @adam-singer, @caass, and @zbirenbaum)

---

nativelink-macro/Cargo.toml line 10 at r3 (raw file):

[dependencies]
# TODO(allada) We currently need to pin these to specific version.

This comment can be removed as the deps are no longer pinned.

---

nativelink-util/src/channel_body_for_tests.rs line 1 at r3 (raw file):

// Copyright 2023 The NativeLink Authors. All rights reserved.

Copyright 2023 should be 2024. This is the case in most of the repository, though, so it's not a big deal. Likewise for other new files.

[adam-singer]

Reviewable status: 1 of 5 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), and 2 discussions need to be resolved (waiting on @adam-singer, @caass, @jhpratt, and @zbirenbaum)

---

nativelink-util/Cargo.toml line 17 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I'll leave that up to @adam-singer to address in a separate PR.

sg, can we add tokio-rs/console#571 / tokio-rs/console#576 in the comment and file a ticket to track it or will renovate pick up pin versions like this?

[aaronmondal]

Reviewable status: 1 of 5 LGTMs obtained, and pending CI: Installation / ubuntu-22.04, pre-commit-checks (waiting on @adam-singer, @caass, @jhpratt, and @zbirenbaum)

---

nativelink-util/Cargo.toml line 17 at r2 (raw file):

Previously, adam-singer (Adam Singer) wrote…

sg, can we add tokio-rs/console#571 / tokio-rs/console#576 in the comment and file a ticket to track it or will renovate pick up pin versions like this?

I'm not sure whether renovate picks this up. I'll create a ticket just in case once we land (leaving this comment open for now).

Since we're AFAIK blocked on #1164, there is a chance that a new version will be available at the time we land this PR.

---

nativelink-util/src/channel_body_for_tests.rs line 1 at r3 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

Copyright 2023 should be 2024. This is the case in most of the repository, though, so it's not a big deal. Likewise for other new files.

Done. I also sent #1172 to fix this in other files.

[jhpratt]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 2 of 5 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) (waiting on @adam-singer, @caass, and @zbirenbaum)

[allada]

The metrics refactor is in, so this PR should be able to land.

[caass]

Reviewed 25 of 25 files at r5, all commit messages.
Reviewable status: 3 of 5 LGTMs obtained, and all files reviewed, and pending CI: integration-tests (20.04), integration-tests (22.04), and 1 discussions need to be resolved (waiting on @adam-singer and @zbirenbaum)

---

BUILD.bazel line 31 at r5 (raw file):

        "@crates//:clap",
        "@crates//:futures",
        "@crates//:hyper-1.4.1",

Wait, should we be specifying our crates like this everywhere? I didn't realize we could put version qualifiers on them. Can we put feature flags on them, too?

---

Cargo.toml line 51 at r5 (raw file):

clap = { version = "4.5.9", features = ["derive"] }
futures = "0.3.30"
hyper = { version = "1.4.1" }

Minor, is there a reason we don't use the shorthand here?

---

nativelink-store/BUILD.bazel line 55 at r5 (raw file):

        "@crates//:hex",
        "@crates//:http-body",
        "@crates//:hyper-0.14.30",

Oh, I see -- we're still using the old hyper here.

[aaronmondal]

Reviewable status: 3 of 5 LGTMs obtained, and 34 of 37 files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, windows-2022 / stable (waiting on @adam-singer and @zbirenbaum)

---

BUILD.bazel line 31 at r5 (raw file):

Previously, caass (Cass Fridkin) wrote…

Wait, should we be specifying our crates like this everywhere? I didn't realize we could put version qualifiers on them. Can we put feature flags on them, too?

AFAIK this is only useful for using two crates of the same version, but there aren't specific feature variants.

You can query available crates like so:

bazel query @crates//...

[aaronmondal]

Reviewed 7 of 34 files at r2, 2 of 9 files at r3, 3 of 4 files at r4, 22 of 25 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: 3 of 5 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, windows-2022 / stable (waiting on @adam-singer and @zbirenbaum)

[aaronmondal]

-@adam-singer -@zbirenbaum

Reviewable status: 3 of 3 LGTMs obtained, and all files reviewed, and pending CI: Installation / macos-13, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04