-
Notifications
You must be signed in to change notification settings - Fork 109
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
Migrate to hyper 1.x, axum 0.7.x, tonic 0.12.x #1155
Conversation
e1b3c8d
to
19fa92e
Compare
9b25792
to
de1ae18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 spawn
s, 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.
There was a problem hiding this 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 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
:
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
spawn
s, 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
The metrics refactor is in, so this PR should be able to land. |
3b247da
to
b8f0ec0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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//...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
https://www.youtube.com/watch?v=7Twnmhe948A&ab_channel=Kontor.TV
This change is