Integrate Java SDK with new service protocol negotiation mechanism #318

tillrohrmann · 2024-05-16T20:28:39Z

This commit lets the Java SDK extract the accepted ServiceDiscoveryProtocolVersions
from the accept header of the discovery request and sends a correspondingly encoded
response back if the endpoint supports the service discovery protocol version.

Likewise the SDK is now extracting the service protocol version from the content type
header when the service is invoked. If the endpoint does not support the specified
protocol version, then it will reject the request with a status code 415.

We are no longer sending the protocol version via the message headers
of the start message. This commit removes this logic.

This fixes #317.

…1898426 1898426 Update min/max of protocol version fields in endpoint_manifest_schema 08871e0 Introduce service.Version and discovery.Version enums 2f3e461 Workflow api changes (restatedev#91) 1fa71a5 Add versioning info (restatedev#90) git-subtree-dir: sdk-core/src/main/service-protocol git-subtree-split: 1898426fc98c16d704068594cd54394912845ff7

github-actions · 2024-05-16T20:31:58Z

Test Results

645 tests - 21 611 ✅ - 46 2m 14s ⏱️ - 1m 2s
48 suites ± 0 9 💤 ± 0
48 files ± 0 25 ❌ +25

For more details on these failures, see this check.

Results for commit 32e0b7d. ± Comparison against base commit 1c4b4e7.

This pull request removes 44 and adds 23 tests. Note that renamed tests count towards both.

dev.restate.e2e.JavaAwaitTimeoutTest ‑ Test Awaitable#await(Duration)
dev.restate.e2e.JavaAwakeableTest ‑ generate(IngressClient)
dev.restate.e2e.JavaCancelInvocationTest ‑ cancel blocked invocation on AWAKEABLE
dev.restate.e2e.JavaCancelInvocationTest ‑ cancel blocked invocation on CALL
dev.restate.e2e.JavaCancelInvocationTest ‑ cancel blocked invocation on SLEEP
dev.restate.e2e.JavaCoordinatorWithNodeReceiverServiceToServiceCallTest ‑ synchronousCall(IngressClient)
dev.restate.e2e.JavaErrorsTest ‑ Test calling method that fails terminally
dev.restate.e2e.JavaErrorsTest ‑ Test calling method that fails terminally multiple times
dev.restate.e2e.JavaErrorsTest ‑ Test invocations are retried until they succeed
dev.restate.e2e.JavaErrorsTest ‑ Test propagate failure from another service
…

dev.restate.e2e.JavaAwaitTimeoutTest ‑ initializationError
dev.restate.e2e.JavaAwakeableTest ‑ initializationError
dev.restate.e2e.JavaCancelInvocationTest ‑ initializationError
dev.restate.e2e.JavaCoordinatorWithNodeReceiverServiceToServiceCallTest ‑ initializationError
dev.restate.e2e.JavaErrorsTest ‑ initializationError
dev.restate.e2e.JavaKafkaIngressTest ‑ initializationError
dev.restate.e2e.JavaRequestSigningTest ‑ initializationError
dev.restate.e2e.JavaSampleWorkflowTest ‑ initializationError
dev.restate.e2e.JavaServiceToServiceCallTest ‑ initializationError
dev.restate.e2e.JavaSimpleSleepTest ‑ initializationError
…

♻️ This comment has been updated with latest results.

tillrohrmann · 2024-05-16T20:48:31Z

The PR is still missing the Lambda handler part.

sdk-core/src/main/java/dev/restate/sdk/core/ServiceProtocol.java

slinkydeveloper · 2024-05-17T06:07:58Z

sdk-http-vertx/src/main/java/dev/restate/sdk/http/vertx/HttpRequestFlowAdapter.java

+  HttpRequestFlowAdapter(
+      HttpServerRequest httpServerRequest, Protocol.ServiceProtocolVersion serviceProtocolVersion) {
+    assert serviceProtocolVersion == Protocol.ServiceProtocolVersion.V1
+        : "Unsupported service protocol version";


Not sure why we need this assert here. I would remove it.

The idea is to make bumping the service protocol version a conscious decision which might need to touch the encoder/decoder as well.

I've changed it into a IllegalArgumentException.

Not sure this helps, when you bump the protocol you need to do it in the endpoint already, this just adds another point to touch when bumping. Also this class it's a package private class anyway, and only instantiated in the endpoint class.

Also this piece of code most likely won't change with next protocol updates, but only the internal encoders/decoders. If it will, it's a big enough change that we won't forget to update the request handlers. On the other side, we will most likely forget updating these assertions here (for lambda even worse, we don't have automated tests that check the lambda sdks work fine...)

This would be the place where one passes the serviceProtocolVersion to the encoder/decoders, I guess. Atm they only support V1 so I didn't bother to pass it through. If you like, then I can also do this instead.

I've removed this check from the code.

slinkydeveloper · 2024-05-17T06:08:04Z

sdk-http-vertx/src/main/java/dev/restate/sdk/http/vertx/HttpResponseFlowAdapter.java

+  HttpResponseFlowAdapter(
+      HttpServerResponse httpServerResponse,
+      Protocol.ServiceProtocolVersion serviceProtocolVersion) {
+    assert serviceProtocolVersion == Protocol.ServiceProtocolVersion.V1
+        : "Unsupported service protocol version";


Not sure why we need this assert here. I would remove it.

sdk-http-vertx/src/main/java/dev/restate/sdk/http/vertx/RequestHttpServerHandler.java

tillrohrmann · 2024-05-17T09:08:29Z

Thanks for the review @slinkydeveloper. I've addressed your comments and added the support for the lambda case. PTAL.

tillrohrmann · 2024-05-17T09:09:51Z

The CounterTest should be failing because it needs restatedev/restate#1526

sdk-http-vertx/src/main/java/dev/restate/sdk/http/vertx/RequestHttpServerHandler.java

slinkydeveloper · 2024-05-17T09:29:17Z

sdk-http-vertx/src/main/java/dev/restate/sdk/http/vertx/HttpRequestFlowAdapter.java

+  HttpRequestFlowAdapter(
+      HttpServerRequest httpServerRequest, Protocol.ServiceProtocolVersion serviceProtocolVersion) {
+    assert serviceProtocolVersion == Protocol.ServiceProtocolVersion.V1
+        : "Unsupported service protocol version";


Not sure this helps, when you bump the protocol you need to do it in the endpoint already, this just adds another point to touch when bumping. Also this class it's a package private class anyway, and only instantiated in the endpoint class.

tillrohrmann · 2024-05-17T13:33:02Z

I've tested the e2e tests locally and they passed with restatedev/restate#1526.

slinkydeveloper

Looks great! Thank you so much for taking care of this!

sdk-lambda/src/main/java/dev/restate/sdk/lambda/LambdaFlowAdapters.java

…aders This commit lets the Java SDK extract the accepted ServiceDiscoveryProtocolVersions from the accept header of the discovery request and sends a correspondingly encoded response back if the endpoint supports the service discovery protocol version. Likewise the SDK is now extracting the service protocol version from the content type header when the service is invoked. If the endpoint does not support the specified protocol version, then it will reject the request with a status code 415.

We are no longer sending the protocol version via the message headers of the start message. This commit removes this logic. This fixes restatedev#317.

tillrohrmann · 2024-05-19T18:13:50Z

Thanks for the review @slinkydeveloper. Merging this PR once GHA gives green light (modulo failing e2e and CounterTest).

tillrohrmann added 4 commits May 16, 2024 21:09

Merge commit '45c404a421551e330db20a9ff1a6b238c7fa9b09' into issues/317

1c8ae6b

Make Java SDK work with updated service-protocol

182c521

Add support for service discovery protocol version selection

6ca6504

tillrohrmann requested a review from slinkydeveloper May 16, 2024 20:28

slinkydeveloper requested changes May 17, 2024

View reviewed changes

tillrohrmann force-pushed the issues/317 branch from 6ccda42 to a502045 Compare May 17, 2024 09:08

tillrohrmann requested a review from slinkydeveloper May 17, 2024 09:08

slinkydeveloper requested changes May 17, 2024

View reviewed changes

tillrohrmann force-pushed the issues/317 branch 2 times, most recently from a1ce667 to b28ac1f Compare May 17, 2024 09:34

tillrohrmann requested a review from slinkydeveloper May 17, 2024 09:35

tillrohrmann force-pushed the issues/317 branch from b28ac1f to d37b475 Compare May 17, 2024 12:00

slinkydeveloper approved these changes May 17, 2024

View reviewed changes

sdk-lambda/src/main/java/dev/restate/sdk/lambda/LambdaFlowAdapters.java Outdated Show resolved Hide resolved

sdk-lambda/src/main/java/dev/restate/sdk/lambda/LambdaFlowAdapters.java Outdated Show resolved Hide resolved

tillrohrmann added 3 commits May 19, 2024 20:12

Remove the outdated version protocol check

ecde439

We are no longer sending the protocol version via the message headers of the start message. This commit removes this logic. This fixes restatedev#317.

Replace Deployment* with EndpointManifestSchema

32e0b7d

tillrohrmann force-pushed the issues/317 branch from d37b475 to 32e0b7d Compare May 19, 2024 18:13

tillrohrmann merged commit 32e0b7d into restatedev:main May 19, 2024
1 of 4 checks passed

tillrohrmann deleted the issues/317 branch May 19, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Java SDK with new service protocol negotiation mechanism #318

Integrate Java SDK with new service protocol negotiation mechanism #318

tillrohrmann commented May 16, 2024

github-actions bot commented May 16, 2024 •

edited

Loading

tillrohrmann commented May 16, 2024

slinkydeveloper May 17, 2024

tillrohrmann May 17, 2024

tillrohrmann May 17, 2024

slinkydeveloper May 17, 2024

slinkydeveloper May 17, 2024 •

edited

Loading

tillrohrmann May 17, 2024 •

edited

Loading

tillrohrmann May 17, 2024

slinkydeveloper May 17, 2024

tillrohrmann commented May 17, 2024

tillrohrmann commented May 17, 2024

slinkydeveloper May 17, 2024

tillrohrmann commented May 17, 2024

slinkydeveloper left a comment

tillrohrmann commented May 19, 2024 •

edited

Loading

Integrate Java SDK with new service protocol negotiation mechanism #318

Integrate Java SDK with new service protocol negotiation mechanism #318

Conversation

tillrohrmann commented May 16, 2024

github-actions bot commented May 16, 2024 • edited Loading

Test Results

tillrohrmann commented May 16, 2024

slinkydeveloper May 17, 2024

Choose a reason for hiding this comment

tillrohrmann May 17, 2024

Choose a reason for hiding this comment

tillrohrmann May 17, 2024

Choose a reason for hiding this comment

slinkydeveloper May 17, 2024

Choose a reason for hiding this comment

slinkydeveloper May 17, 2024 • edited Loading

Choose a reason for hiding this comment

tillrohrmann May 17, 2024 • edited Loading

Choose a reason for hiding this comment

tillrohrmann May 17, 2024

Choose a reason for hiding this comment

slinkydeveloper May 17, 2024

Choose a reason for hiding this comment

tillrohrmann commented May 17, 2024

tillrohrmann commented May 17, 2024

slinkydeveloper May 17, 2024

Choose a reason for hiding this comment

tillrohrmann commented May 17, 2024

slinkydeveloper left a comment

Choose a reason for hiding this comment

tillrohrmann commented May 19, 2024 • edited Loading

github-actions bot commented May 16, 2024 •

edited

Loading

slinkydeveloper May 17, 2024 •

edited

Loading

tillrohrmann May 17, 2024 •

edited

Loading

tillrohrmann commented May 19, 2024 •

edited

Loading