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

Use GET for service discovery #1613

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

slinkydeveloper
Copy link
Contributor

Fix #1259

@slinkydeveloper
Copy link
Contributor Author

This one doesn't break anything, as both SDKs are not checking the method used to invoke service discovery.

@igalshilman
Copy link
Contributor

@slinkydeveloper Looks good to me.
Also using GET allows caching, which is okay for us (I think)

But just as an abundance of caution, let me ask @jackkleeman or @pcholakov if this might have any effect on lambda? (i'd assume not, but just to make sure)

@pcholakov
Copy link
Contributor

I'd assume not, provided the SDK deals with it the same regardless of the invocation transport. Since Lambda just takes a JSON payload blob natively, we just shove the request into an API Gateway HTTP request event, and then unpack it on the handler side. If I'm reading this[^1] correctly, we disregard the method altogether when we pass the inbound request from the Lambda-specific to the generic invoke handler in the TS SDK.

[1] https://github.com/restatedev/sdk-typescript/blob/main/packages/restate-sdk/src/endpoint/lambda_handler.ts#L55-L110

@pcholakov
Copy link
Contributor

Ditto for the Java SDK, just had a quick look; I don't think anything at all cares about the HTTP method specified in the payload.

@igalshilman
Copy link
Contributor

Thanks a lot @pcholakov.
Then LGTM!

@tillrohrmann
Copy link
Contributor

@slinkydeveloper Looks good to me. Also using GET allows caching, which is okay for us (I think)

Could caching become a problem if one deploys a new service container and then one would get a stale discovery response?

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. +1 for merging.

@slinkydeveloper slinkydeveloper merged commit 3e36e4d into restatedev:main Jun 11, 2024
4 checks passed
@slinkydeveloper slinkydeveloper deleted the issues/1259 branch June 11, 2024 10:18
@slinkydeveloper
Copy link
Contributor Author

Could caching become a problem if one deploys a new service container and then one would get a stale discovery response?

In theory the discovery response is immutable for a given service endpoint. In any case, none of our components do caching of GET requests, at most it will be some 3rd party proxy, and if that becomes an issue we can fix it by using a combination of headers, e.g. https://stackoverflow.com/questions/10927766/force-http-request-to-not-cache

tillrohrmann pushed a commit that referenced this pull request Jun 11, 2024
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.

Service discovery should use GET
4 participants