Implement generic grpc handlers #4295

rbair23 · 2022-11-15T02:08:04Z

The classes defined in this PR are pretty close to production ready and represent a best-effort at how to do this properly. Please review carefully:

The use of metrics (is this right?). I was unable to reuse the metrics classes in services today because the mono-service module cannot be referenced from the hedera-app module (mono-service is not a proper module today)
I added Helidon to the build. I may be able to revert and use plain Netty with minimal changes (I did it once before in the POC). Or we can keep Helidon. I'm torn on it.
I updated a few library version numbers in settings.gradle.kts

This implementation is plumbed into IngestWorkflow and QueryWorkflow, but both those workflows are no-ops at the moment (implemented by different issues). As such, this code doesn't actually do anything yet, other than parse incoming bytes and returning empty bytes.

Signed-off-by: Richard Bair <[email protected]>

codecov · 2022-11-15T19:47:21Z

Codecov Report

Base: 0.00% // Head: 96.45% // Increases project coverage by +96.45% 🎉

Coverage data is based on head (5bbb13e) compared to base (a86e228).
Patch coverage: 83.73% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff               @@
##             develop    #4295       +/-   ##
==============================================
+ Coverage           0   96.45%   +96.45%     
- Complexity         0    15029    +15029     
==============================================
  Files              0     1010     +1010     
  Lines              0    42810    +42810     
  Branches           0     4338     +4338     
==============================================
+ Hits               0    41294    +41294     
- Misses             0      764      +764     
- Partials           0      752      +752

Impacted Files	Coverage Δ
...-app/src/main/java/com/hedera/node/app/Hedera.java	`0.00% <0.00%> (ø)`
.../node/app/workflows/ingest/IngestWorkflowImpl.java	`0.00% <0.00%> (ø)`
...ra/node/app/workflows/query/QueryWorkflowImpl.java	`0.00% <0.00%> (ø)`
.../main/java/com/hedera/node/app/SessionContext.java	`100.00% <100.00%> (ø)`
...a/com/hedera/node/app/grpc/GrpcServiceBuilder.java	`100.00% <100.00%> (ø)`
...va/com/hedera/node/app/grpc/KnownLengthStream.java	`100.00% <100.00%> (ø)`
...main/java/com/hedera/node/app/grpc/MethodBase.java	`100.00% <100.00%> (ø)`
.../java/com/hedera/node/app/grpc/NoopMarshaller.java	`100.00% <100.00%> (ø)`
...ain/java/com/hedera/node/app/grpc/QueryMethod.java	`100.00% <100.00%> (ø)`
...va/com/hedera/node/app/grpc/TransactionMethod.java	`100.00% <100.00%> (ø)`
... and 1000 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tinker-michaelj · 2022-11-15T22:51:48Z

The use of metrics (is this right?). I was unable to reuse the metrics classes in services today because the mono-service module cannot be referenced from the hedera-app module (mono-service is not a proper module today)

I think the exact names of the metrics are important for the Elastic ingest pipelines; but not an urgent concern.

I updated a few library version numbers in settings.gradle.kts

There may be a version conflict with the io.grpc version coming in via com.hedera.hashgraph:hedera-protobuf-java-api, which is at 1.39.0; I guess we should just bump that one!

tinker-michaelj

LGTM!

Neeharika-Sompalli

LGTM ! Few Javadocs missing and have a question on TODOs.

hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/QueryHandler.java

hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/TransactionHandler.java

hedera-node/hedera-app/src/main/java/com/hedera/node/app/Hedera.java

hedera-node/hedera-app/src/main/java/com/hedera/node/app/SessionContext.java

...node/hedera-app/src/main/java/com/hedera/node/app/workflows/prehandle/PreHandleWorkflow.java

mhess-swl

Overall looks great!! Just a couple small comments

hedera-node/hedera-app/src/test/java/com/hedera/node/app/grpc/QueryMethodTest.java

hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/ingest/IngestWorkflow.java

hedera-node/hedera-app/src/itest/java/grpc/GrpcTest.java

hedera-node/hedera-app/src/main/java/com/hedera/node/app/Hedera.java

hedera-node/hedera-app/src/test/java/com/hedera/node/app/grpc/TransactionMethodTest.java

Signed-off-by: Richard Bair <[email protected]>

rbair23 · 2022-11-16T05:32:59Z

Yay, grpc package has 100% Line, Method, and Class coverage :-). There is one case that is only covered in integration tests, which bothers me, and I don't have any tests for metrics. I will add the tests for metrics with issue #4293

Signed-off-by: Richard Bair <[email protected]>

rbair23 · 2022-11-16T06:09:54Z

Yay, grpc package has 100% Line, Method, and Class coverage :-). There is one case that is only covered in integration tests, which bothers me, and I don't have any tests for metrics. I will add the tests for metrics with issue #4293

OK, added the test case that bothered me. Sonar is still going to complain because I didn't add tests for Hedera.java, but this is intentional, as this class is not "real" or ready for prime-time. Refactoring that and testing it will come, though probably mainly through itest, which doesn't look like is used for coverage...

Signed-off-by: Richard Bair <[email protected]>

sonarqubecloud · 2022-11-16T13:37:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

84.6% Coverage
0.0% Duplication

hendrikebbers · 2022-11-16T13:07:00Z

settings.gradle.kts

            // Use when you need logging
-            bundle("logging", listOf("log4j-api", "log4j-core"))
+            bundle("logging", listOf("log4j-api", "log4j-core", "log4j-slf4j", "slf4j-api"))


I would not define a logging bundle that contains all this dependencies since some should not be available at compiletime.

From my point of view we should only have a completive dependency to slf4j-api in all new modules. log4j-slf4j and log4j-api / log4j-core should be defined as runtime-only dependencies.

I assume we have the same problem with some other bundles. That's why I'm not a fan of bundles: You can not do the compile / runtime split.

hendrikebbers · 2022-11-16T13:17:21Z

buildSrc/src/main/kotlin/com.hedera.hashgraph.jpms-modules.gradle.kts

@@ -59,6 +64,9 @@ extraJavaModuleInfo {
    automaticModule("io.prometheus:simpleclient_httpserver", "io.prometheus.simpleclient.httpserver")

    automaticModule("j2objc-annotations-1.3.jar", "j2objc.annotations")
+    automaticModule("io.perfmark:perfmark-api", "perfmark.api")


io.perfmark:perfmark-api:0.26.0 defines io.perfmark as automatic module name. We should use the same.

Suggested change

automaticModule("io.perfmark:perfmark-api", "perfmark.api")

automaticModule("io.perfmark:perfmark-api", "io.perfmark")

hendrikebbers · 2022-11-16T13:18:06Z

buildSrc/src/main/kotlin/com.hedera.hashgraph.jpms-modules.gradle.kts

@@ -59,6 +64,9 @@ extraJavaModuleInfo {
    automaticModule("io.prometheus:simpleclient_httpserver", "io.prometheus.simpleclient.httpserver")

    automaticModule("j2objc-annotations-1.3.jar", "j2objc.annotations")
+    automaticModule("io.perfmark:perfmark-api", "perfmark.api")
+
+    automaticModule("org.eclipse.microprofile.health:microprofile-health-api", "microprofile.health.api")


ARHG! Unbelievable that such project does not have at least an automatic module name. Created an issue: microprofile/microprofile-health#322

hendrikebbers · 2022-11-16T13:22:24Z

buildSrc/src/main/kotlin/com.hedera.hashgraph.jpms-modules.gradle.kts

@@ -30,6 +30,11 @@ extraJavaModuleInfo {

    automaticModule("io.grpc:grpc-api", "io.grpc.api")
    automaticModule("io.grpc:grpc-netty", "io.grpc.netty")
+    automaticModule("io.grpc:grpc-stub", "grpc.stub")


protocolbuffers/protobuf#11005 created

hendrikebbers · 2022-11-16T13:22:51Z

hedera-node/hedera-app-spi/build.gradle.kts

@@ -21,4 +21,9 @@ description = "Hedera Application - SPI"

 dependencies {
    implementation(libs.hapi)
+    implementation(libs.jsr305.annotation)


compileOnly should be used

hendrikebbers · 2022-11-16T13:37:17Z

hedera-node/hedera-app/src/main/java/com/hedera/node/app/grpc/GrpcServiceBuilder.java

+     * @return A reference to the builder.
+     */
+    public @Nonnull GrpcServiceBuilder query(@Nonnull String methodName) {
+        if (Objects.requireNonNull(methodName).isBlank()) {


should we use the helper methods of the platform commons module for such checks? Still do not now if we want to expose such functionality in the platform or recreate it in the services.

hendrikebbers · 2022-11-16T13:39:33Z

hedera-node/hedera-app/src/main/java/com/hedera/node/app/grpc/GrpcServiceBuilder.java

+            @Nonnull String serviceName,
+            @Nonnull IngestWorkflow ingestWorkflow,
+            @Nonnull QueryWorkflow queryWorkflow) {
+        this.ingestWorkflow = Objects.requireNonNull(ingestWorkflow);


I like to do such runtime null checks next to the compiletime annotation checks. Why should define this as best practice since I've seen several places in the code that does not do such checks

hendrikebbers · 2022-11-16T13:40:22Z

hedera-node/hedera-app/src/main/java/com/hedera/node/app/grpc/MethodBase.java

+     * Create a new instance.
+     *
+     * @param serviceName a non-null reference to the service name
+     * @param methodName a non-null reference to the method name


metrics param is missing

hendrikebbers · 2022-11-16T13:41:05Z

hedera-node/hedera-app/src/main/java/com/hedera/node/app/grpc/MethodBase.java

+    public void invoke(
+            @Nonnull ByteBuffer requestBuffer,
+            @Nonnull StreamObserver<ByteBuffer> responseObserver) {
+        try {


Nullcheck for responseObserver is missing

hendrikebbers · 2022-11-16T13:45:25Z

hedera-node/hedera-app/src/main/java/module-info.java

+    requires io.helidon.webserver.http2;
+    requires io.helidon.grpc.server;
+    requires io.helidon.grpc.core;
+    requires jsr305;


Suggested change

requires jsr305;

requires static jsr305;

rbair23 added 3 commits November 14, 2022 17:54

Generic gRPC handler for all transactions and queries.

ce18b30

Signed-off-by: Richard Bair <[email protected]>

Apply spotless formatting

ac3fd76

Signed-off-by: Richard Bair <[email protected]>

Apply spotless formatting

9b00d24

Signed-off-by: Richard Bair <[email protected]>

rbair23 requested review from kimbor, jasperpotts, netopyr, hendrikebbers, tinker-michaelj, Neeharika-Sompalli and mhess-swl November 15, 2022 02:08

rbair23 requested a review from a team as a code owner November 15, 2022 02:08

rbair23 requested a review from JeffreyDallas November 15, 2022 02:08

rbair23 added the CI:UnitTests label Nov 15, 2022

Fix integration test to start the server for testing

a377051

Signed-off-by: Richard Bair <[email protected]>

tinker-michaelj approved these changes Nov 15, 2022

View reviewed changes

Neeharika-Sompalli reviewed Nov 15, 2022

View reviewed changes

mhess-swl approved these changes Nov 16, 2022

View reviewed changes

rbair23 added 3 commits November 15, 2022 21:09

Fill out integration test cases for gRPC

9f9a4ac

Signed-off-by: Richard Bair <[email protected]>

Apply code formatting

cc7e0d3

Signed-off-by: Richard Bair <[email protected]>

Address code review comments

705c058

Signed-off-by: Richard Bair <[email protected]>

rbair23 added the CI:FinalChecks label Nov 16, 2022

rbair23 mentioned this pull request Nov 16, 2022

Pattern for the new modules #4264

Merged

Increase coverage

5bbb13e

Signed-off-by: Richard Bair <[email protected]>

Merge branch 'develop' into 04203-Implement-generic-grpc-handlers

fac984b

Signed-off-by: Richard Bair <[email protected]>

rbair23 merged commit aa552a4 into develop Nov 16, 2022

rbair23 deleted the 04203-Implement-generic-grpc-handlers branch November 16, 2022 13:28

rbair23 self-assigned this Nov 16, 2022

This was linked to issues Nov 16, 2022

Implement generic gRPC handlers #4203

Closed

Update the grpc libs to a new version #4238

Closed

hendrikebbers reviewed Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement generic grpc handlers #4295

Implement generic grpc handlers #4295

rbair23 commented Nov 15, 2022

codecov bot commented Nov 15, 2022 •

edited

Loading

tinker-michaelj commented Nov 15, 2022

tinker-michaelj left a comment

Neeharika-Sompalli left a comment

mhess-swl left a comment

rbair23 commented Nov 16, 2022

rbair23 commented Nov 16, 2022

sonarqubecloud bot commented Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

hendrikebbers Nov 16, 2022

	automaticModule("io.perfmark:perfmark-api", "perfmark.api")
	automaticModule("io.perfmark:perfmark-api", "io.perfmark")

Implement generic grpc handlers #4295

Implement generic grpc handlers #4295

Conversation

rbair23 commented Nov 15, 2022

codecov bot commented Nov 15, 2022 • edited Loading

Codecov Report

tinker-michaelj commented Nov 15, 2022

tinker-michaelj left a comment

Choose a reason for hiding this comment

Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

mhess-swl left a comment

Choose a reason for hiding this comment

rbair23 commented Nov 16, 2022

rbair23 commented Nov 16, 2022

sonarqubecloud bot commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2022 •

edited

Loading