Skip to content

Move PartitionProcessorRpcClient in restate_types, and rename it InvocationClient.#3211

Merged
slinkydeveloper merged 2 commits into
restatedev:mainfrom
slinkydeveloper:decouple-pp-invocation-client
May 5, 2025
Merged

Move PartitionProcessorRpcClient in restate_types, and rename it InvocationClient.#3211
slinkydeveloper merged 2 commits into
restatedev:mainfrom
slinkydeveloper:decouple-pp-invocation-client

Conversation

@slinkydeveloper
Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper commented Apr 30, 2025

The InvocationClient should be completely orthogonal from the transport, and should be easy to mock.

We're gonna use this same client in the Admin API soon for #3186 followups, for propagating errors in the admin api.

}

/// This trait provides the functionalities to interact with Restate invocations.
pub trait InvocationClient {
Copy link
Copy Markdown
Contributor Author

@slinkydeveloper slinkydeveloper Apr 30, 2025

Choose a reason for hiding this comment

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

This trait should really be ortoghonal from the transport, and in theory the types used here should not implement serde, nor any other serialization stuff. Coupling the types here couples the implementation too, and makes hard the future evolution (see the not so nice serde_hacks we ended up with in invocation and other modules...).

For now this is a simple refactoring of the existing code, but perhaps @muhamadazmy that's something you can keep in mind when working on network serialization.

In Java and other dynamic languages, this problem would be solved by overriding the default type serializer/deserializer class, allowing to evolve the "domain data structure" separately from its serialized representation. My understanding is that in rust the best we can do is proxy data structures, or using remote derive like https://serde.rs/remote-derive.html, but maybe there's some alternative out there for this...

Copy link
Copy Markdown
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR @slinkydeveloper. The changes look good to me. +1 for merging.

Comment thread crates/ingress-http/src/handler/workflow.rs
Comment thread crates/types/src/invocation/client.rs Outdated
Comment thread crates/types/src/invocation/client.rs
Comment thread crates/types/src/invocation/client.rs Outdated
@slinkydeveloper slinkydeveloper force-pushed the decouple-pp-invocation-client branch from d96abd5 to 235d6f8 Compare May 5, 2025 07:26
…cationClient.

The InvocationClient should be completely orthogonal from the transport, and should be easy to mock.

We're gonna use this same client in the Admin API soon.
@slinkydeveloper slinkydeveloper force-pushed the decouple-pp-invocation-client branch from 235d6f8 to 3e2cf2c Compare May 5, 2025 08:35
@slinkydeveloper slinkydeveloper merged commit 8c8d64a into restatedev:main May 5, 2025
27 checks passed
@slinkydeveloper slinkydeveloper deleted the decouple-pp-invocation-client branch May 5, 2025 09:11
@github-actions github-actions Bot locked and limited conversation to collaborators May 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants