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

feat: create Service trait #2920

Merged
merged 12 commits into from Sep 8, 2022

Conversation

tomkarw
Copy link
Contributor

@tomkarw tomkarw commented Jul 23, 2022

Closes #2853.

@tomkarw tomkarw force-pushed the feat-create-service-trait branch 2 times, most recently from bfb20c3 to 99ed036 Compare July 27, 2022 21:45
src/client/client.rs Outdated Show resolved Hide resolved
@seanmonstar
Copy link
Member

Ok, I see. You could leave the tower-service dependency in for now, since the work to transfer this client stuff is in a separate PR, and then this PR is just replacing the server side. Then when both PRs are done, we can chop out of the Cargo.toml. I think that'll be the least complicated as all this movement happens.

@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 1, 2022

I might be doing something wrong, but it's impossible to just replace the server part. It would require maintaining duplicates of most parts of src/service and some parts src/common, one with stuff that implements tower_service::Service and one with hyper::service::Service. The clean sweep throughout the repo was the closest I got to compiling, working code. Trying to do anything local to src/server just keeps leaking to other parts of the repo.

@seanmonstar I could use some guidance here. I saw you did some work on removing tcp feature and parts of src/client. Should I maybe wait till client part is completely gone? I think clean sweep remove of tower_service dependency is the best way to move this MR forward.

@tomkarw tomkarw force-pushed the feat-create-service-trait branch 2 times, most recently from 1c71ec6 to e7c6b5a Compare August 18, 2022 18:56
@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 18, 2022

Alright, with the help of cross-cutting removals of code Sean have done to the client module, this PR was way easier to do.

Looks good to me now, @seanmonstar I'd ask for CI run and if it's all green (except for miri) a review.

@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 19, 2022

Dumb mistake, I fought the compiler for so long I forgot to run the tests once the project finally compiled.

Locally, all the tests (including examples) pass now.

There is only one reference to tower left in the whole project, and it's in client/conn/mod.rs here. We could easily make SendRequest implement hyper::service::Service instead and remove the dependency all together. It would be a clean sweep of tower/tower-service from the project.

@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 19, 2022

I can do fmt + check or test, but I seem to be uncapable of remembering to run all 3 of them before pushing 😄

@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 19, 2022

Alright, couldn't stop myself from removing the last tower reference from client::conn::SendRequest. The repo is now all free of the dependency. If you think that's wrong, I can revert the last commit.

I'm certain pipelines will pass, so I'll wait for the decision and CR.

@tomkarw
Copy link
Contributor Author

tomkarw commented Aug 19, 2022

Juuust one more CI trigger.

src/service/service.rs Outdated Show resolved Hide resolved
@tomkarw tomkarw requested review from sfackler and seanmonstar and removed request for seanmonstar and sfackler August 19, 2022 19:16
Cargo.toml Outdated Show resolved Hide resolved
src/client/conn/mod.rs Show resolved Hide resolved
src/proto/h1/dispatch.rs Outdated Show resolved Hide resolved
src/proto/h2/server.rs Show resolved Hide resolved
@tomkarw
Copy link
Contributor Author

tomkarw commented Sep 1, 2022

@seanmonstar Could you address #2920 (comment) and #2920 (comment), and I'll move this PR forward?

@tomkarw tomkarw reopened this Sep 5, 2022
@tomkarw
Copy link
Contributor Author

tomkarw commented Sep 5, 2022

@seanmonstar I introduced changes from the discussion threads.

LGTM, let's wrap and merge this as soon as possible, as the merge conflicts on this branch are killing me 😜

@tomkarw
Copy link
Contributor Author

tomkarw commented Sep 7, 2022

CI failed on a doc-test, this should be fixed now.

@seanmonstar seanmonstar merged commit fee7d36 into hyperium:master Sep 8, 2022
@seanmonstar
Copy link
Member

Thanks so so so much! I know this dragged on a while, but it turns out there were tricky things for us to figure out (and conflicts as all sorts of other things got yanked).

@tomkarw tomkarw deleted the feat-create-service-trait branch September 8, 2022 22:37
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.

Create hyper::Service trait
3 participants