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

Add documentation about service type and service contract #78

Merged
merged 3 commits into from
Jul 27, 2023
Merged

Conversation

slinkydeveloper
Copy link
Contributor

Fix #60

Copy link
Contributor

@gvdongen gvdongen left a comment

Choose a reason for hiding this comment

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

Reads nicely! Few small suggestions.

```protobuf
message GreetingRequest {
Person person = 1 [(dev.restate.ext.field) = KEY];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to just put the tag on two fields?
In that case I think we should mention that... Some users might depend on whatever they get from external systems and might not be able to choose the message format. But I guess they can put an unkeyed translator service in between...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some doc to specify that there can be only one key field


Once you have the contract, the SDK uses it to generate the code to encode/decode messages and the interface to implement the service. You can import contract of other services, and the SDK will generate clients to invoke them.

The contract can also be used to generate gRPC/Connect clients to invoke Restate services from your webapp, mobile app, legacy system or in general from any system outside Restate services through the [ingress](./ingress.md). You can check out the [gRPC](https://grpc.io/docs/languages/) and [Connect](https://connect.build/docs/introduction) documentation for more info on the available clients.
Copy link
Contributor

Choose a reason for hiding this comment

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

A more general thought. When we use the word Connect I always wonder whether it's not clearer to use plain HTTP because from the perspective of the end user, connect is an implementation detail right?
But I agree that if we mention it anywhere, this should be the place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, plain HTTP doesn't mean much if you don't specify how to put the request in the http envelope, in particular for protobuf users might be strange to just read plain HTTP.

For me Connect is not the implementation detail, but rather HTTP is, as it's the transport of Connect. Also we use the Connect name all around the docs, in particular in the ingress docs. I would keep it as it is.

docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved

* Time-consuming operations, such as sleep, lock the service instance for the entire operation, hence they won't allow other enqueued invocations to be executed.
* Keyed service invocations can produce deadlocks when using request/response calls. When this happens, the keys remain locked and the system can't process any more requests. In this situation you'll have to unblock the keys manually by [cancelling invocations](./deployment-operations/manage-invocations.md#cancel-an-invocation). Some example cases:
* Cross deadlock between service A and B: A calls B, and B calls A, both using same keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is also a cycle, just with only 2 participants... But I am being difficult...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on your definition of cycle: for me a cycle is when I end up in a deadlock due to only my own actions, while this cross deadlock can happen when two services are independently doing their own thing, perhaps from different ingress requests, and cross-invoking each other they end up in the deadlock. But I might be fuzzy on the names here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I forgot about that option

docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved
@gvdongen
Copy link
Contributor

Fixes #5

@gvdongen gvdongen linked an issue Jul 21, 2023 that may be closed by this pull request
Copy link
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.

LGTM. I had few minor comments.

docs/service_contract.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved
docs/service_type.md Outdated Show resolved Hide resolved

Carefully ponder whether a service should be a singleton, given it executes all the invocations serially. If not properly used, it can end up being a quite significant source of resource contention for your application.

## Unkeyed service
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether we shouldn't start with unkeyed services in our description because they are the simplest type of services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure, it's strange to start a documentation page from the negation of another type. This page is not a "starter" page, so starting from what's easy is less of a concern IMO. It also reads better this way, because explains service types as "one variation of the other".

I would keep as it is and perhaps when reorganizing the docs #66 we could give another pass at this.

slinkydeveloper and others added 2 commits July 21, 2023 13:17
Co-authored-by: Till Rohrmann <[email protected]>
Co-authored-by: Giselle van Dongen <[email protected]>
@gvdongen gvdongen merged commit fa5f40f into main Jul 27, 2023
@gvdongen gvdongen deleted the issues/60 branch July 27, 2023 15:01
@gvdongen
Copy link
Contributor

Merging this to take it along in the restructuring effort.

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.

Write docs on Restate-specific parts of protobuf service definition Doc: Write services section
3 participants