gRPC: fix request context propagation #17946

michalszynkiewicz · 2021-06-16T09:58:14Z

No description provided.

extensions/grpc/runtime/src/main/java/io/quarkus/grpc/runtime/health/GrpcHealthEndpoint.java

...untime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingExecutionHandler.java

...rc/main/java/io/quarkus/grpc/runtime/supports/context/GrpcRequestContextGrpcInterceptor.java

Ladicek · 2021-06-16T10:18:41Z

Looks good on the first sight, but I don't truly understand how gRPC is integrated / how a gRPC request is processed to provide any meaningful review.

quarkus-bot · 2021-06-16T11:04:17Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `689b9ef`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Gradle Tests - JDK 11	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Gradle Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Maven Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Amazon	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Cache		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data3	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging2	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

quarkus-bot · 2021-06-16T12:55:56Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `1c94d97`

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/grpc-hibernate-reactive

✖ com.example.reactive.ReactiveServiceTest.shouldWatchAndAddMultipleTimes - More details - Source on GitHub

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.BlockingMutinyTest.shouldAddViaBidi - More details - Source on GitHub

quarkus-bot · 2021-06-16T13:25:45Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `ddc3e3c`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	`Reclaim Disk Space`	⚠️ Check →	Logs	Raw logs

quarkus-bot · 2021-06-16T14:16:29Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `61b4bb8`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Gradle Tests - JDK 11	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Gradle Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	`Build`	⚠️ Check →	Logs	Raw logs
✖	Maven Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
⌛	Maven Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Amazon	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Cache	`Download Maven Repo`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging2	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2	`Download Maven Repo`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security3	`Extract Maven Repo`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 849 - More details - Source on GitHub

...untime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingExecutionHandler.java

...ntime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingServerInterceptor.java

quarkus-bot · 2021-06-17T10:28:56Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `e242827`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	`Cache Maven Repository`	⚠️ Check →	Logs	Raw logs

...ration-tests/grpc-hibernate-reactive/src/main/java/com/example/reactive/ReactiveService.java

Ladicek

Looks good, baring one little comment about testing. Also, maybe squash the commits now?

(I originally asked for a separate commit to see the changes you did after our Friday call, but we should probably squash before merging.)

Ladicek · 2021-06-21T08:18:42Z

...ration-tests/grpc-hibernate-reactive/src/main/java/com/example/reactive/ReactiveService.java

+                            if (contextChecker.requestContextId() != contextId) {
+                                throw new RuntimeException("Different context for onCompleted and RawTestService#bidi method");
+                            }
+                            if (!requestContext.isActive()) {
+                                throw new RuntimeException("Request context not active for `onCompleted`");
+                            }


Exception messages here don't correspond to present code. There's no onCompleted here, and there's no RawTestService#bidi.

Sorry to be such a pain :-)

Thanks for catching it, I wanted to be quick, sorry.

quarkus-bot · 2021-06-21T09:22:06Z

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building `4c4c032`

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	JVM Tests - JDK 11	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	`Build`	⚠️ Check →	Logs	Raw logs
✖	Native Tests - Data3	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security1	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2	`Build`	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

gsmet · 2021-06-21T09:53:42Z

This looks a bit too involved to get backported but I'm not sure how critical the fix is? Given there's no backport label, I understand you didn't plan to backport it?

michalszynkiewicz · 2021-06-21T10:04:19Z

I think it should be backported, I'll add the label.

quarkus-bot · 2021-06-21T10:05:54Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `18ed189`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	⚠️ Check →	Logs	Raw logs

manovotn

From my limited understanding PoV, this looks fairly good :-)

quarkus-bot · 2021-06-21T12:38:27Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `dcb9e97`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Native Tests - Security3	`Download Maven Repo`	⚠️ Check →	Logs	Raw logs

gsmet · 2021-06-21T13:02:47Z

@mkouba I let you have a look and merge it if it's fine for you.

michalszynkiewicz · 2021-06-21T13:34:45Z

The tests for security should not in any way be impacted by this change and they failed with downloading maven repo :| :

Error: The HTTP request timed out after 00:01:40.
Error: Exit code 1 returned from process: file name '/home/runner/runners/2.278.0/bin/Runner.PluginHost', arguments 'action "GitHub.Runner.Plugins.Artifact.DownloadArtifact, Runner.Plugins"'.

michalszynkiewicz · 2021-06-21T13:35:19Z

@gsmet I don't think it makes sense to rerun these tests, WDYT?

gsmet · 2021-06-21T14:56:50Z

Agreed, merging.

gsmet · 2021-06-21T14:57:39Z

Ah no, I'll let @mkouba have a look if he can and merge tomorrow if we don't hear from him as it would be better to have it in 2.0.0.Final.

...ntime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingServerInterceptor.java

quarkus-bot · 2021-06-22T09:24:44Z

Failing Jobs - Building `8b9c8b2`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 16	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.OpenTelemetryTestCase.testClientTracing line 308 - More details - Source on GitHub

📦 integration-tests/scala

✖ io.quarkus.scala.maven.it.ScalaDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 23 - More details - Source on GitHub

✖ io.quarkus.scala.maven.it.ScalaRemoteDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 25 - More details - Source on GitHub

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/scala

✖ io.quarkus.scala.maven.it.ScalaDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 23 - More details - Source on GitHub

✖ io.quarkus.scala.maven.it.ScalaRemoteDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 25 - More details - Source on GitHub

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/scala

✖ io.quarkus.scala.maven.it.ScalaDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 23 - More details - Source on GitHub

✖ io.quarkus.scala.maven.it.ScalaRemoteDevModeIT.testThatTheApplicationIsReloadedOnScalaChange line 25 - More details - Source on GitHub

michalszynkiewicz requested review from cescoffier, mkouba and Ladicek June 16, 2021 09:58

quarkus-bot bot added the area/grpc gRPC label Jun 16, 2021

Ladicek reviewed Jun 16, 2021

View reviewed changes

extensions/grpc/runtime/src/main/java/io/quarkus/grpc/runtime/health/GrpcHealthEndpoint.java Outdated Show resolved Hide resolved

Ladicek reviewed Jun 16, 2021

View reviewed changes

...untime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingExecutionHandler.java Show resolved Hide resolved

Ladicek reviewed Jun 16, 2021

View reviewed changes

...rc/main/java/io/quarkus/grpc/runtime/supports/context/GrpcRequestContextGrpcInterceptor.java Outdated Show resolved Hide resolved

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from 689b9ef to 1c94d97 Compare June 16, 2021 11:00

michalszynkiewicz force-pushed the fix-grpc-request-scope branch 2 times, most recently from ddc3e3c to 61b4bb8 Compare June 16, 2021 13:25

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from 61b4bb8 to d4ebd6b Compare June 16, 2021 14:13

cescoffier requested changes Jun 16, 2021

View reviewed changes

michalszynkiewicz force-pushed the fix-grpc-request-scope branch 2 times, most recently from e242827 to c62fe3b Compare June 17, 2021 10:28

michalszynkiewicz requested a review from cescoffier June 17, 2021 10:28

cescoffier approved these changes Jun 20, 2021

View reviewed changes

Ladicek reviewed Jun 21, 2021

View reviewed changes

...ration-tests/grpc-hibernate-reactive/src/main/java/com/example/reactive/ReactiveService.java Outdated Show resolved Hide resolved

Ladicek approved these changes Jun 21, 2021

View reviewed changes

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from 56e7d9c to 4c4c032 Compare June 21, 2021 07:48

Ladicek reviewed Jun 21, 2021

View reviewed changes

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from 4c4c032 to 18ed189 Compare June 21, 2021 09:16

michalszynkiewicz added the triage/backport? label Jun 21, 2021

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from 18ed189 to dcb9e97 Compare June 21, 2021 10:26

Ladicek approved these changes Jun 21, 2021

View reviewed changes

manovotn approved these changes Jun 21, 2021

View reviewed changes

mkouba reviewed Jun 21, 2021

View reviewed changes

...ntime/src/main/java/io/quarkus/grpc/runtime/supports/blocking/BlockingServerInterceptor.java Show resolved Hide resolved

gRPC: fix request context propagation

8b9c8b2

michalszynkiewicz force-pushed the fix-grpc-request-scope branch from dcb9e97 to 8b9c8b2 Compare June 22, 2021 07:24

mkouba approved these changes Jun 22, 2021

View reviewed changes

michalszynkiewicz added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 22, 2021

gsmet merged commit 71f9dc5 into quarkusio:main Jun 22, 2021

quarkus-bot bot added this to the 2.1 - main milestone Jun 22, 2021

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 22, 2021

gsmet modified the milestones: 2.1 - main, 2.0.0.Final Jun 22, 2021

gsmet removed the triage/backport? label Jun 22, 2021

gRPC: fix request context propagation #17946

gRPC: fix request context propagation #17946

Conversation

michalszynkiewicz commented Jun 16, 2021

Ladicek commented Jun 16, 2021

quarkus-bot bot commented Jun 16, 2021 • edited Loading

Failing Jobs - Building 689b9ef

quarkus-bot bot commented Jun 16, 2021 • edited Loading

Failing Jobs - Building 1c94d97

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/grpc-hibernate-reactive

📦 integration-tests/grpc-hibernate

quarkus-bot bot commented Jun 16, 2021 • edited Loading

Failing Jobs - Building ddc3e3c

quarkus-bot bot commented Jun 16, 2021 • edited Loading

Failing Jobs - Building 61b4bb8

Test Failures

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

quarkus-bot bot commented Jun 17, 2021 • edited Loading

Failing Jobs - Building e242827

Ladicek left a comment

Choose a reason for hiding this comment

Ladicek Jun 21, 2021

Choose a reason for hiding this comment

michalszynkiewicz Jun 21, 2021

Choose a reason for hiding this comment

quarkus-bot bot commented Jun 21, 2021 • edited Loading

Failing Jobs - Building 4c4c032

gsmet commented Jun 21, 2021

michalszynkiewicz commented Jun 21, 2021

quarkus-bot bot commented Jun 21, 2021 • edited Loading

Failing Jobs - Building 18ed189

manovotn left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jun 21, 2021 • edited Loading

Failing Jobs - Building dcb9e97

gsmet commented Jun 21, 2021

michalszynkiewicz commented Jun 21, 2021

michalszynkiewicz commented Jun 21, 2021

gsmet commented Jun 21, 2021

gsmet commented Jun 21, 2021

quarkus-bot bot commented Jun 22, 2021

Failing Jobs - Building 8b9c8b2

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/opentelemetry

📦 integration-tests/scala

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/scala

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/scala

quarkus-bot bot commented Jun 16, 2021 •

edited

Loading

Failing Jobs - Building `689b9ef`

quarkus-bot bot commented Jun 16, 2021 •

edited

Loading

Failing Jobs - Building `1c94d97`

quarkus-bot bot commented Jun 16, 2021 •

edited

Loading

Failing Jobs - Building `ddc3e3c`

quarkus-bot bot commented Jun 16, 2021 •

edited

Loading

Failing Jobs - Building `61b4bb8`

quarkus-bot bot commented Jun 17, 2021 •

edited

Loading

Failing Jobs - Building `e242827`

quarkus-bot bot commented Jun 21, 2021 •

edited

Loading

Failing Jobs - Building `4c4c032`

quarkus-bot bot commented Jun 21, 2021 •

edited

Loading

Failing Jobs - Building `18ed189`

quarkus-bot bot commented Jun 21, 2021 •

edited

Loading

Failing Jobs - Building `dcb9e97`

Failing Jobs - Building `8b9c8b2`