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.

Sign up for GitHub

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

Jump to bottom

feat: update libp2p-server and metric example to use hyper 1.0 version #5038

Closed
wants to merge 1 commit into from
Closed

feat: update libp2p-server and metric example to use hyper 1.0 version #5038

wants to merge 1 commit into from

Conversation

getong
Copy link

@getong getong commented Dec 26, 2023

Description

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@getong getong changed the title update metric example to use hyper 1.0 version feat: update metric example to use hyper 1.0 version Dec 26, 2023
@getong getong changed the title feat: update metric example to use hyper 1.0 version feat: update libp2p-server and metric example to use hyper 1.0 version Dec 26, 2023
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 27, 2023

This pull request has merge conflicts. Could you please resolve them @getong? 🙏

@getong getong force-pushed the update-hyper-1.0 branch 2 times, most recently from a6e7322 to 75cff0c Compare December 30, 2023 22:22
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 1, 2024

This pull request has merge conflicts. Could you please resolve them @getong? 🙏

@thomaseizinger
Copy link
Contributor

Sorry for the delay, I should have some time mid-January to review this.

@getong
Copy link
Author

getong commented Jan 3, 2024

How about enabling ci first, I'd like to see what ci runs here.

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 11, 2024

This pull request has merge conflicts. Could you please resolve them @getong? 🙏

thomaseizinger
thomaseizinger reviewed Jan 14, 2024
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you! I've left some feedback :)

examples/metrics/Cargo.toml Outdated Show resolved Hide resolved
examples/metrics/src/http_service.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this got even more verbose with the update to hyper 1.0. Can we perhaps simplify this by migrating it to a higher-level library like axum?

Copy link
Author

Choose a reason for hiding this comment

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

Axum is using hyper under the hood, but axum is still not reached 1.0, and hyper now is stable to be used as 1.0.
I think it does not need to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are using many library that are < 1.0. I am more concerned with the boilerplate around setting up an HTTP services with just using hyper. As mentioned above, with the update to 1.0, it seems that that is even more verbose than pre 1.0.

We already use axum in other places. Would you be willing to migrate libp2p-server and metric-example to use latest axum (which I believe already uses hyper 1.0 under the hood).

Copy link
Author

Choose a reason for hiding this comment

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

I have no time to fix it by using axum.

Copy link
Member

Choose a reason for hiding this comment

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

@drHuangMHT do you wanna give this a try with axum?

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 15, 2024

This pull request has merge conflicts. Could you please resolve them @getong? 🙏

@getong getong force-pushed the update-hyper-1.0 branch from 63e5333 to be617aa Compare March 18, 2024 14:32
@jxs jxs mentioned this pull request Mar 21, 2024
Merged
4 tasks
@jxs
Copy link
Member

jxs commented Mar 25, 2024

friendly ping @getong

@getong getong force-pushed the update-hyper-1.0 branch 7 times, most recently from 3eecb6a to 5e7d79b Compare March 28, 2024 14:40
@getong
feat: update metric example to use hyper 1.0 version
817fb8b 
update hyper version to 1.0 in libp2p-server
@getong getong force-pushed the update-hyper-1.0 branch from 5e7d79b to 817fb8b Compare March 28, 2024 14:47
@getong
Copy link
Author

getong commented Mar 28, 2024

Quite a long time, with small change updated now.

mergify bot pushed a commit that referenced this pull request Apr 11, 2024
@drHuangMHT
deps: move libp2p-server and metrics example to use axum 0.7
042174e 
Move from `hyper` to `axum` for `libp2p-server` and metrics example.
Running in parallel with #5038.

Pull-Request: #5246.
@getong getong closed this Apr 14, 2024
@getong getong deleted the update-hyper-1.0 branch July 30, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs jxs left review comments

@thomaseizinger thomaseizinger thomaseizinger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@getong @thomaseizinger @jxs