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: unbox the ClaimService and Scoped futures #653

Merged

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Feb 27, 2023

We can unbox the future type in tower layers if we implement future ourselves. For how I landed on the implementation for ClaimService, see: https://github.com/tower-rs/tower/blob/master/guides/building-a-middleware-from-scratch.md

For what inspired the enum solution for the Scoped future, see: https://github.com/tower-rs/tower/blob/master/tower/src/load_shed/future.rs#L13-L63

Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks @oddgrd ❤️

It also seems small enough to merge. Btw, do you have rewrk stats for this maybe?

@oddgrd
Copy link
Contributor Author

oddgrd commented Feb 27, 2023

Thanks! Yes, the difference is quite small, so small that I would need to do more tests to verify its significance. One could argue that this is a bit overkill, but I learned a lot about how to do this and I think we can see more gains in layers like ShuttleAuthLayer. So I'm happy whether we merge this or not. 👍

@chesedo chesedo merged commit fb7c5ae into shuttle-hq:main Feb 27, 2023
@chesedo
Copy link
Contributor

chesedo commented Feb 27, 2023

Nah, I would definitely want to convert all of them eventually. At a scale of thousands this won't be an overkill as these are called for almost every request

@oddgrd
Copy link
Contributor Author

oddgrd commented Feb 27, 2023

That's good because I had fun converting them 😄

@oddgrd oddgrd deleted the feat/implement-future-for-claimservice branch February 27, 2023 19:14
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.

2 participants