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

Use hyper 1.0 and tonic 0.12 in storage broker #9234

Merged
merged 9 commits into from
Oct 2, 2024

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Oct 2, 2024

Fixes #9231 .

Upgrade hyper to 1.4.0 and use hyper 1.4 instead of 0.14 in the storage broker, together with tonic 0.12. The two upgrades go hand in hand.

Thanks to the broker being independent from other components, we can upgrade its hyper version without touching the other components, which makes things easier.

Copy link

github-actions bot commented Oct 2, 2024

5084 tests run: 4898 passed, 0 failed, 186 skipped (full report)


Flaky tests (1)

Postgres 14

Code coverage* (full report)

  • functions: 31.3% (7487 of 23882 functions)
  • lines: 49.5% (60108 of 121323 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
3109c4b at 2024-10-02T21:17:51.259Z :recycle:

@arpad-m arpad-m requested a review from a team as a code owner October 2, 2024 05:59
storage_broker/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I think this is fine, but I would add a comment or assertion/expect/unreachable why we don't expect serve to fail anymore.

use of http_body_util::Either can be done later.

@koivunej
Copy link
Member

koivunej commented Oct 2, 2024

Hmm I think we should change "Fixes" to "Cc" or something similar in PR description, because we will still have hyper pre-1.0, or open another issue for that?

@koivunej
Copy link
Member

koivunej commented Oct 2, 2024

@arpad-m
Copy link
Member Author

arpad-m commented Oct 2, 2024

I think we should change "Fixes" to "Cc" or something similar in PR description, because we will still have hyper pre-1.0, or open another issue for that?

IMO the issue is mainly about updating tonic, and the parts about updating hyper are to update hyper in enough places that a tonic update is possible. This is accomplished by this PR.

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
storage_broker/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I think the current impl does not serve more than one connection at a time, but the old one did. I think we should had had some tests fail for this.

@koivunej
Copy link
Member

koivunej commented Oct 2, 2024

I am still unsure why not a lot of test failures with regress tests if storage_broker only served a single connection (pageserver or safekeeper).

Prevents panics:

hyper-1.4.1/src/common/time.rs:37:17: You must supply a timer.
@arpad-m arpad-m merged commit 1b176fe into main Oct 2, 2024
83 checks passed
@arpad-m arpad-m deleted the arpad/broker_tonic_hyper_update branch October 2, 2024 22:48
arpad-m added a commit that referenced this pull request Oct 3, 2024
Follow-up of #9234 to give hyper 1.0 the version-free name, and the
legacy version of hyper the one with the version number inside. As we
move away from hyper 0.14, we can remove the `hyper0` name piece by
piece.

Part of #9255
VladLazar added a commit that referenced this pull request Oct 3, 2024
VladLazar added a commit that referenced this pull request Oct 3, 2024
@VladLazar VladLazar mentioned this pull request Oct 3, 2024
5 tasks
VladLazar added a commit that referenced this pull request Oct 3, 2024
erikgrinaker pushed a commit that referenced this pull request Oct 4, 2024
Follow-up of #9234 to give hyper 1.0 the version-free name, and the
legacy version of hyper the one with the version number inside. As we
move away from hyper 0.14, we can remove the `hyper0` name piece by
piece.

Part of #9255
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.

dep: upgrade tonic + hyper
4 participants