This repository has been archived by the owner on Mar 11, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Go support for CloudState User Functions #5
Go support for CloudState User Functions #5
Changes from 10 commits
52dd4dd
ea5217b
a69533c
cb84071
82e554f
83b5294
5365861
a84b2ee
a8fdc37
d234659
801f67c
14373da
accb0f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more typical to call this struct
Config
. As these values are not options that I can tell - they are required, right? For true options (like timeouts) it's usually better to use the functional option pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Not according to the protocol defined in cloudstate/entity.proto. I followed the requirements there. But even if some of them are optional and some mandatory, naming it Config makes sense, right?
Config
instead ofOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting. I thought the service_name was required, at least. Since this is used by the proxy to forward commands/events to other services, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment these fields? It's not clear to me at first reading what they should be set to. In the example it only sets
Service
so what are the other ones for? Also should the paths be relative to the source code directory or something else?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this struct and implementation with invoking
Descriptor()
directly on theMessage
to get the descriptors? Aka https://godoc.org/github.com/golang/protobuf/descriptor#Message. This should behave similarly to the Java implementation - we shouldn't have to load the proto files ourselves, given we already have the compiledpb.go
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point and also one I iterated for some time. This type is used with
cloudState.Register
where a user registers an event sourced entity like so:For the Service, like in the Java Implementation, I would preferably use a type safe reference to a (Service)Descriptor. The Go gRPC implementation does not export them unfortunately. So to get the Service and any other dependent descriptors the user either provides their names or any of the messages used for the service or those dependent messages.
For the shopping cart, this would be one of the messages used like
shoppingcart.AddLineItem
orshoppingcart.Cart
which I found irritating from a users perspective.The
DescriptorConfig
in this terms lets the user choose how to register the service and dependent descriptors. I see that this is not optimal and gives too many degrees of freedom from the users perspective.I repeat here how that would look like:
Cart
Do you see any other ways to configure a gRPC Service and their dependent descriptors @jsravn ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found https://github.com/jhump/protoreflect, which seems to support retrieving the descriptors from the linked in pb.go files. Although I haven't tested it myself. It also has logic for loading directly from proto files, which we could use instead of implementing it ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godoc.org/github.com/jhump/protoreflect/desc#FileDescriptor.FindService for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen protoreflect and I had the impression I could not get the descriptor this way. It also seems quite a big dependency to depend on tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called RegisterEventSourcedEntity to distinguish from future register calls for other types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Host and port should be set when creating the
CloudState
instance. This makes them explicit and let's users pick the best way to handle those values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was guided here by
io.cloudstate.javasupport.CloudStateRunner
and I don't know what CloudState here expects. It seems for me the API surface here is minimised deliberately but I have to check what was the intention by CloudState.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I raised this on gitter as well just now. I think a library should just have a single well defined API for applications to interact with. If we have an implicit API via something like environment variables, it can get quite confusing what's going on. I think only the application (whatever owns
main.go
) should decide how to interact with its environment - either via command line args or environment variables. This is typically handled by a library like cobra or kingpin, which can automatically merge environment variables and command line parameters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be a golang distinction from Java? I think it's more typical to handle all your command line options/env inside main.go, versus loading these environment things elsewhere in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whereas the user function support library acts as a library or framework, I had the impression to be more on the side of a framework where the API surface is quite limited or reduced so to say and user functions can be implemented without too much ceremony. Having it mentioned in the chat, James or Viktor might pick up on this too as it was not yet discussed yet explicitly.
As we have now these questions where it could be a distinction between Java and Go or any other implementation, it seems it raises perhaps a question that is not answered by the expectations CloudState would have off of a support library and should be defined by it.
Otherwise, I totally agree with your point of having the owner of main.go the sole authority to decide how environment or any other configuration that lets bootstrap the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure
host
is necessary here. In K8s users will almost always just want to listen at:port
so the container listens on the external interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above for line 111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should avoid logging in a library.
Maybe we should have a
Status
function that returns the state of the cloudstate instance? If users want to inspect whether things are running, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments above and discussion in cloudstateio/cloudstate#130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding a
Status
function; on the last contributors call @viktorklang brought the idea to have metadata available to the proxy from the user library out of band, so the proxy gets informed about the user functions state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on that @jroper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to get the value of
EventSourced
from the service descriptor?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't. Like for the other comments about a type safe way to get the service descriptor, we can't (see below the example for _EventSourced_serviceDesc). I explained this also in the then Draft - PR: cloudstateio/cloudstate#103 (comment) on how to access this information. The comment also mentions a few partly open Issues on the gRPC project for Go.
event_sourced.pb.go
@jsravn do you see any other way to do that? Tho, I've seen the https://github.com/gogo/protobuf project and that it is more open in this regard, but I have not tried it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think gogo is only protobuf 2? It's been a while since I looked at it.
It does seem like golang protobuf is fairly anemic, unfortunately.
But I think we can do this with protoreflect:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my prior message regarding
DescriptorConfig
- we shouldn't need to parse the proto file ourselves, as far as I can tell.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't. Generated .pb.go files register their FileDescriptors globally through
proto.RegisterFile
andproto.FileDescriptor
returns just that registered FileDescriptor. We just decompress it and ourunpackFile
function unmarshals it into aFileDescriptorProto
.See above my comment about how to resolve dependent descriptors tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, didn't realize that - I thought it was loading from the filesystem. I think https://github.com/jhump/protoreflect could simplify things a bit though.