-
Notifications
You must be signed in to change notification settings - Fork 85
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
H-3383: Implement harpc
service trait
#5283
Conversation
how would we do codec? that’s still a big-ish question.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5283 +/- ##
==========================================
+ Coverage 19.66% 20.18% +0.51%
==========================================
Files 510 510
Lines 17156 17300 +144
Branches 2546 2546
==========================================
+ Hits 3374 3492 +118
- Misses 13761 13770 +9
- Partials 21 38 +17 ☔ View full report in Codecov by Sentry. |
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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.
Looks good so far, very exciting that we see a service now!
A few comments mainly to clarify a few things. I'm not sure how useful the metadata will be initially because we won't have a versioned API, yet.
|
||
use crate::Service; | ||
|
||
pub trait ServiceDelegate<S, C> { |
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.
Can we add a documentation for this trait please?
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.
see: 350295c
|
||
impl Service for Account { | ||
type ProcedureId = AccountProcedureId; | ||
type Procedures = HList![CreateAccount]; |
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.
Why we cannot use tuples 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.
We could, but I found that a HList
(or similar h-list implementation) is just a lot easier to work with. With tuples you have some unique problems/constraints, like:
- to implement them you need to use a macro, the code is very repetitive
- you have an upper limit (like 16). You can circumvent that limit by using more traits / a proxy trait, but that is just very messy in general and hard to decipher.
That's why I semi recently-ish pivoted to use HList
from frunk
for these kind of things, instead of using tuples. It's a lot easier to write and reason about and the HList!
and hlist!
macro make it easy to construct these types. (also the crate itself has some cool things like coproduct, but that's beside the point).
I found the code to just be a lot more readable on the implementation side, with a bit of a negative (because now you're compelled to use a type-level macro) but found that the tradeoffs were good enough, if you disagree please let me know and I'll try to use tuples 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.
We can stick with HList for now so we can make progress here, but generally, I prefer language-constructs (such as tuples). I don't see why construction of HList
is easier when in a constant scenario such as defining Procedure
. In addition, the documentation is much easier to read when looking at tuples and if we use a macro to implement a trait on each tuple elements that's fine. Something along these lines:
// Potential user-defined implementation if required
impl Trait for CreateAccount {...}
impl<T: Trait> Trait for (T,) { ... }
impl<T: Trait, U: Trait> Trait for (T,U) { ... }
while the Trait
bound might not be needed.
Let's keep it or now, but let's also consider changing it later. Generally, we don't need to create procedures dynamically I guess? If we build it dynamically we could use frunk
as a helper to construct a tuple.
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.
Yes. The reason I like this approach more is that it allows for a clean separation of concerns.
It allows us to have two different traits, one for just the list, another one for the trait itself and it doesn't require a macro to implement everything.
I also found that during dynamically building up a list it's a bit nicer.
I also fully get your point! In a perfect would we would have variable sized tuples we could implement traits over.
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.
I also fully get your point! In a perfect would we would have variable sized tuples we could implement traits over.
Variadic generics or at least variadic tuples would be a game changer for many things 🚀
/// Represents the removal information for a procedure or service. | ||
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
pub struct Removal { | ||
/// The version at which the procedure/service will be removed. | ||
pub version: Version, | ||
/// The reason for removal. | ||
pub reason: Option<&'static str>, | ||
} |
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.
I don't really understand this. How I can get information for a removed procedure?
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.
It's about when something is scheduled for removal, like on v6.0 this will be removed (a future version).
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.
I don't think this is needed. A deprecation warning should be good enough for that. If a function is stable in v3
, then it's deprecated in v4
, then it's expected to be removed later.
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.
you are right, done in 6551827
_: &S, | ||
_: CreateAccount, |
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.
Can we add names to variables 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.
see: d3cdbb9
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.
Happy to keep Removal
in, but also happy if it would go away 😉
Let's defer HList
->tuple
(discussion and potential switch) to a later point when we have more services.
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph |
representative_read_entity
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
scaling_read_entity_complete_one_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
scaling_read_entity_linkless
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10000 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
🌟 What is the purpose of this PR?
This implements the
harpc
service trait. This is the machinery a user (or proc-macro) is required to implement. While this may not look like much, it actually took quite a bit longer to implement, due to considerations of how to do encoding of payload and having a working example.The example
examples/account.rs
shows what an example would look like.This service trait machinery can be split into separate primary parts:
axum
, we use atarpc
like approach, where the user defines a trait that he then implements. This trait is split into two distinct roles with the same signature.Server
andClient
the code for the client can be easily generated (if wanted) and only the server side needs to be implemented by the user.ServiceDelegate
, the service delegate is there to provide a bridge between the typed trait interface and the tower service. it takes just a handful of arguments and then depending on the required argument decodes them. As shown in the example implementing this per hand is quite straight forward, but it would also be possible to do this via machine generated code quite easily, although it would most likely require some sort of procedural macro.Service
andProcedure
trait. Those are not strictly needed, but allow us to, at a later stage, verify some additional information, such as: the current version vs the requested version, procedures that are part of it, and most importantly it allows us to have a bidirectional type level link between the payload (procedure) and every service, through the use of marker traits. TheProcedure
trait and implementation are not strictly needed, but included here as they allow us to provide better documentation for the user (e.g. when was this method added, etc.)Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR: