Initial gRPC implementation and samples #35

dturanski · 2017-10-11T18:00:28Z

Resolves #32

dturanski · 2017-10-11T18:10:28Z

function-sidecar/src/main/java/io/sk8s/sidecar/GrpcProperties.java

+	/**
+	 * The gRPC server port.
+	 */
+	private int port=10382;


Apparently gRPC has no default port. This one is 'gR' decoded.

>>> "%d%d"%(ord('g'),ord('R')) '10382'

ericbottard

Still need to actually run this, but here is a first review pass

ericbottard · 2017-10-12T07:30:23Z

function-proto/pom.xml

+			</plugin>
+		</plugins>
+	</build>
+</project>


Missing newline

I have newline in my local. I'm not sure what's going on.

ericbottard · 2017-10-12T07:31:23Z

function-proto/src/main/proto/fntypes.proto

+}
+
+/*
+ * TODO: Reply can be Empty for sink behavior. Maybe move this can be a separate RPC.


The "Maybe move..." sentence does not make sense

ericbottard · 2017-10-12T07:31:51Z

function-proto/src/main/proto/function.proto

+
+service StringFunction {
+  rpc Call(fntypes.Request) returns (fntypes.Reply) {}
+}


I'm not able to fix this for some reason

ericbottard · 2017-10-12T07:36:34Z

function-sidecar/src/test/java/io/sk8s/sidecar/GrpcIntegrationTests.java

+public class GrpcIntegrationTests {
+	@Autowired Dispatcher dispatcher;
+
+	@Ignore


Hmm, this doesn't test much, does it ? :)

ericbottard · 2017-10-12T07:39:14Z

function-sidecar/src/test/java/io/sk8s/sidecar/GrpcProfileTests.java

+		public void start() throws IOException {
+			server.start();
+
+			Runtime.getRuntime().addShutdownHook(new Thread() {


This feels a bit dirty. Can't cleanup be managed at the test level, rather than the whole JVM's?

Not sure. I took this from sample code on the gRPC developers guide

ericbottard · 2017-10-12T07:42:18Z

samples/grpc/.gitignore

@@ -0,0 +1,2 @@
+proto/
+proto/


Double entry

ericbottard · 2017-10-12T09:21:57Z

I think there is something phony going on with the doubly nested @Configuration classes.
This is what I see in the sidecar log for the echo function (not even involving gRPC normally). The pod does not start correctly.

2017-10-12 09:13:42.101  INFO 1 --- [           main] i.s.sidecar.FunctionSidecarApplication   : The following profiles are active: stdio
2017-10-12 09:13:42.153  INFO 1 --- [           main] ationConfigEmbeddedWebApplicationContext : Refreshing org.springframework.boot.context.embedded.AnnotationConfigEmbeddedWebApplicationContext@6477463f: startup date [Thu Oct 12 09:13:42 GMT 2017]; root of context hierarchy
2017-10-12 09:13:42.770  INFO 1 --- [           main] o.s.b.f.s.DefaultListableBeanFactory     : Overriding bean definition for bean 'dispatcher' with a different definition: replacing [Root bean: class [null]; scope=; abstract=false; lazyInit=false; autowireMode=3; dependencyCheck=0; autowireCandidate=true; primary=false; factoryBeanName=dispatcherConfiguration.GrpcDispatcherConfig.BlockingStubConfiguration; factoryMethodName=dispatcher; initMethodName=null; destroyMethodName=(inferred); defined in class path resource [io/sk8s/sidecar/DispatcherConfiguration$GrpcDispatcherConfig$BlockingStubConfiguration.class]] with [Root bean: class [null]; scope=; abstract=false; lazyInit=false; autowireMode=3; dependencyCheck=0; autowireCandidate=true; primary=false; factoryBeanName=io.sk8s.sidecar.DispatcherConfiguration$StdioDispatcherConfig; factoryMethodName=dispatcher; initMethodName=null; destroyMethodName=(inferred); defined in class path resource [io/sk8s/sidecar/DispatcherConfiguration$StdioDispatcherConfig.class]]

Wondering if the level 2 inner class should not bear @Profile as well

ericbottard · 2017-10-12T09:28:26Z

README.adoc

+
+=== Build the gRPC sample image
+```
+samples/grpc/dockerize


I'd centralize the (non-maven) dockerizations in the dockerize script at the root of the project

So far the main dockerize script is for required components. This one is only required for the grpc sample.

ericbottard · 2017-10-12T09:29:26Z

function-proto/pom.xml

+	<properties>
+		<java.version>1.8</java.version>
+		<protobuf.version>3.3.0</protobuf.version>
+		<grpc.version>1.4.0</grpc.version>


https://twitter.com/aiborisov/status/918286280084959233 :)

ericbottard · 2017-10-12T09:53:38Z

function-proto/pom.xml

+				<artifactId>protobuf-maven-plugin</artifactId>
+				<version>${protobuf.plugin.version}</version>
+				<configuration>
+					<protocArtifact>com.google.protobuf:protoc:3.3.0:exe:${os.detected.classifier}</protocArtifact>


Version numbers in here should use properties defined above

dturanski · 2017-10-16T12:53:48Z

Closing - will create a new PR to merge with master

David Turanski added 2 commits October 11, 2017 13:57

Initial gRPC implementation and samples

473e3a7

Add new line at end

f14b331

dturanski commented Oct 11, 2017

View reviewed changes

ericbottard reviewed Oct 12, 2017

View reviewed changes

David Turanski added 3 commits October 12, 2017 06:54

Add grpc to teardown

c24c7d8

Cleanup

12e05f8

Add consumer group

92cc778

dturanski mentioned this pull request Oct 12, 2017

Fix multiple functions on the same topic #40

Closed

David Turanski added 2 commits October 13, 2017 11:51

Add newlines

58f0b55

Add newlines

f037703

dturanski closed this Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial gRPC implementation and samples #35

Initial gRPC implementation and samples #35

dturanski commented Oct 11, 2017 •

edited

dturanski Oct 11, 2017

ericbottard left a comment

ericbottard Oct 12, 2017

dturanski Oct 13, 2017

ericbottard Oct 12, 2017

ericbottard Oct 12, 2017

dturanski Oct 13, 2017

ericbottard Oct 12, 2017

ericbottard Oct 12, 2017

dturanski Oct 12, 2017

ericbottard Oct 12, 2017

ericbottard commented Oct 12, 2017 •

edited

ericbottard Oct 12, 2017

dturanski Oct 12, 2017

ericbottard Oct 12, 2017

ericbottard Oct 12, 2017

dturanski commented Oct 16, 2017

		@@ -0,0 +1,2 @@
		proto/
		proto/

Initial gRPC implementation and samples #35

Initial gRPC implementation and samples #35

Conversation

dturanski commented Oct 11, 2017 • edited

Choose a reason for hiding this comment

ericbottard left a 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

Choose a reason for hiding this comment

ericbottard commented Oct 12, 2017 • edited

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

dturanski commented Oct 16, 2017

dturanski commented Oct 11, 2017 •

edited

ericbottard commented Oct 12, 2017 •

edited