Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Introduce Ingress service and BackgroundInvoke #19

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented May 30, 2023

@slinkydeveloper slinkydeveloper added this to the 1B milestone May 30, 2023
Copy link
Contributor

@igalshilman igalshilman left a comment

Choose a reason for hiding this comment

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

Thanks @slinkydeveloper, the service definition looks good to me.

service Ingress {
// Invoke a service and don't wait for the response.
// It is guaranteed that the service will be invoked after this method returns.
rpc BackgroundInvoke(InvokeRequest) returns (BackgroundInvokeResponse);
Copy link
Contributor

@igalshilman igalshilman May 30, 2023

Choose a reason for hiding this comment

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

Seems like the TypeScript SDK, and the documentation calls this "one way call", we probably should align to that name here as well.
Once will standardize on the name we'll make a change across the board.

Copy link
Contributor

Choose a reason for hiding this comment

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

You know, thinking about this a bit more, I think that it is distinct enough from the SDK to deserve its own concept.
And since it is under the ingress service, it is okay to imply call it Invoke.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would you be fine with BackgroundInvoke then? The reason I wouldn't call it simply Invoke is that i can foresee in future adding a variant of this method that actually waits for the response.

Copy link
Contributor

Choose a reason for hiding this comment

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

BackgroundInvoke will not be consistent with the SDK and the docs atm.
Lets do simply Invoke for now, because I don't have a better name, and if we would need a different variant we will change and rename.

// this field must contain the serialized Protobuf matching the argument type of the target method.
// When executing requests to the ingress using JSON,
// this field must contain the JSON object representing the argument type of the target method.
bytes argument = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a bit of a pity that this has to be like that, the call site becomes clunky, but that's Protobuf.

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper May 30, 2023

Choose a reason for hiding this comment

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

I've seen in other APIs that it's normal to use bytes fields for nested messages for which you don't have a type definition at compile time, e.g. see the Any type. In any case, for JSON the users will just pass the json object, so the interface will look "natural" to them.

@slinkydeveloper slinkydeveloper merged commit a7ce45d into main Jun 2, 2023
@slinkydeveloper slinkydeveloper deleted the background-invoke branch June 2, 2023 13:06
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