feature/go-support #103
feature/go-support #103
Conversation
…ed to get started on
… back. Send over dependent protos.
…ents and snapshots)
…uild and TCK configuration in place
…entation. Passes the TCK 100%. PoC included for reference; will be removed.
…. removed redundant ServiceInfo initialization.
… CodeReviewComments best practices.
…ith a docker container for *NIX and Windows (prepared).
…tation, snapshot interfaces added, minor cleanups (err flows,...)
…separated by a space.
…ting method if an event is not handled by the implemented EventHandler#HandleEvent
|
Thanks @marcellanz for opening this exciting PR! I'm definitely no Go-expert so it's going to take some time to digest :) Looking forward to having a look at this! :) |
|
@viktorklang
"Go is simple and you should be able to learn it in a very short amount of time"(tm) The most magic stuff happens with reflection; by which we need a bit of because of the dynamic nature of handlers. There is also one or two of sub-optimal things about the gRPC implementation; a ServiceDescriptor is not exposed explicitly by it and so it makes things like the registration of the entity and its service cumbersome.I iterated quite some times over that and hope I found something as expressive as possible without too much implicit magic. Actually the current form is explicit as a user can pass the proto filename; but also one of the proto message types used by the service which is typesafe, as I prefer it, but not to be expected => why "register" one of the services messages types go get a gRPC service registered.
I have both on an issue list on my branch here of what next steps for go-support might be here I'll clean up the proto file handling and its generation to be ready for this PR.
Thanks, I'm looking forward to your feedback. |
|
@marcellanz I took a stab at some of the build integration and proto generation things here: 5b9df7c My Go is definitely not fluent enough :D I'll start looking at the Go-specific code shortly :) I assume(?) that we want to split the go-support up into a library and the shopping-cart example, and put the go-shopping-cart in samples/… ? I'm still not sure about the best way of running the compilation, if we should assume Go is installed or not, or run it in Docker. |
nice, thank you!
Yes, I'd support that and is what I had in my mind also to go forward. The *.proto files would be the only artefacts to share between libraries I think. "go imports" also expect that a go package lives on the edge of a VCS repository
That was my motivation to introduce the Docker supported go compilation; so that the project as a whole can run the TCK and a CloudState developer doesn't need all compilers (Go, "node", Swift,...) installed correctly, which might be tricky, for the project. IMO the docker approach is kind of elegant to liberate the developers environment from that burden. Having a library of a language support implementation would mean a separate repository, yes? It would be the most effective way to go for a Go library as Go's module and package management expects to use VCS endpoints for packages and imports directly. I have also not thought too much about the topic, but it came to me as I would have to provide a canonical "godoc" story which assumes to have a separate repository for the go code. |
I'd be totally open to have a cloudstateio/go-support repository, perhaps we should consider this for all language supports, at least when the CloudState Specification is more formalized, as there is less maintenance burden due to making sweeping changes across the board.
Indeed. I'm not sure how we'll deal with Linux/Mac/Win support for that though. We'll need to figure out the testing matrix at some point.
I'm not sure whether it'd be strictly required or not. There are pros and cons to both models—monolithic builds are easier to overview etc, but having a repo for each language support makes for a cleaner interface between each.
Ideally we should be able to create a native executable using GraalVM native-image to build the TCK and put it in a Docker container for easy reuse, then it'd only be a matter of parameterizing a docker run to verify an implementation.
Good point, I was not aware of this :) @jroper or any other community member might want to weigh in? :) |
This, the godoc story, should not drive such a decision for sure and shouldn't be a priority. It can be done without it but it is more complex than I thought. There is an old and still open issue about that on Go's side golang/go#2381 (comment) where it is still surprisingly non-trivial to produce, just some "bunch of HTML godoc" for a package. |
|
|
Here's a snippet, FWIW, from a sample
|
@marcellanz This works for me.
|
go-support/cloudstate/cloudstate.go
Outdated
| } | ||
|
|
||
| func (r *EntityDiscoveryResponder) registerFileDescriptor(msg descriptor.Message) error { | ||
| fd, _ := descriptor.ForMessage(msg) // this can panic, so we do here |
There was a problem hiding this comment.
@marcellanz Please help me understand your parenthetical note (i.e. // this can panic, so we do here) and in general why we're not performing some sort of error-handling here. I'm trying to reconcile this with some received wisdom, for example, and to give credit where it's due, quoting here from the fine book Go in Practice by Matt Butcher and Matt Farina (Manning Publications), they point out that:
Go assumes that errors will be handled by you, the programmer. If an error occurs and you ignore it, Go doesn’t do anything on your behalf. Not so with panics. When a panic occurs, Go unwinds the stack, looking for handlers for that panic. If no handler is found, Go eventually unwinds all the way to the top of the function stack and stops the program. An unhandled panic will kill your application.
There was a problem hiding this comment.
To my regret, I left quite a few Fatal failures left, just so that I got the full attention during start of development for these situations. They're all moved into proper errors with an update to this PR branch.
I also share your concerns regarding exit via log.Fatalf or panic() a program except for very valid reasons at its startup where it can't start at all. Also libraries should not panic, except perhaps internally.
In this case descriptor.ForMessage does panic by itself and also does not visibly states that by its function name, like using MustXXX. So I decided to let it panic until I had a better way to handle it. I fixed it and recover it know and move it into an error registerFileDescriptorwill return.
| } | ||
|
|
||
| // see EventSourcedServer.Handle | ||
| func (esh *EventSourcedHandler) Handle(server protocol.EventSourced_HandleServer) error { |
There was a problem hiding this comment.
This is minor, but I noticed mixed-casing (in EventSourced_HandleServer) of CamelCase 🐫 and snake_case 🐍 - I mention this only because you've taken great care to make casing consistent throughout. And I speak here, in turn, only to the incredibly opinionated theme (the Go way of doing things) that pervades the Go programming paradigm, such as the chafing requirement for everything being inside a GOPATH, not to mention Upper-casing function names and variables in order to export them out of the default package scope 😵
There was a problem hiding this comment.
I fully support consistent naming. In this case these names are generated by the gRPC compiler plugin protoc-gen-go which uses a mix of CamelCase and snake_case identifiers for messages and fields and AFAIK I can't much about it for now.
There is also an issue that addresses this behaviour on the protobuf support project of Go.
There was a problem hiding this comment.
Ah, I see. No worries in that case, we're good for now 👍
| if err != nil { | ||
| log.Fatalf("unable to Marshal") | ||
| } | ||
| events = append(events, |
There was a problem hiding this comment.
- Did you have any concerns regarding the order in which events are emitted in connection with the receiver? Thinking out loud here, as I haven't stepped through the code with a debugger.
- But thinking specifically to the following two in conjunction: (1)
events := esh.marshalEventsTo(entityValue)and (2) the following (trailing) section infunc (esh *EventSourcedHandler) marshalEventsTo(entityValue reflect.Value) []*any.Any { ... }:
marshal, err := proto.Marshal(message) if err != nil { log.Fatalf("unable to Marshal") } events = append(events, &any.Any{ TypeUrl: fmt.Sprintf("%s/%s", protoAnyBase, proto.MessageName(message)), Value: marshal, }, ) } emitter.Clear()}
return events
There was a problem hiding this comment.
Did you have any concerns regarding the order in which events are emitted
@akramtexas Actually I'm not. This should be completely in sequence of what is stated instruction wise. Since theres is, at the first call shortly discussed, no concurrent data path involved, the EventSourcedHandler has at all times full control over the path how events are flowing around. Do you see any races here?
There was a problem hiding this comment.
@marcellanz Here, too, you have removed my concern. Looks good. For me, the final word on concurrency (in Go!) is the fine book named “Concurrency in Go” by Katherine Cox-Buday, where she has a helpful tip toward the end of the book. FWIW, like so:
“In Go 1.1, a -race flag was added as a flag for most go commands:
$ go test -race mypkg # test the package
$ go run -race mysrc.go # compile and run the program
$ go build -race mycmd # build the command
$ go install -race mypkg # install the package
If you’re a developer and all you need is a more reliable way to detect race conditions, this is really all you need to know. One caveat of using the race detector is that the algorithm will only find races that are contained in code that is exercised. For this reason, the Go team recommends running a build of your application built with the race flag under real-world load. This increases the probability of finding races by virtue of increasing the probability that more code is exercised.”
There was a problem hiding this comment.
@akramtexas yeah the -race flag is of great help for detecting races in code. As a precaution I have been using this flag with the go-support/build/run_go_test_in_docker.sh script
go test -count 1 -race ./...
We might even run the TCK with the flag enabled to get covered here.
There was a problem hiding this comment.
@marcellanz Good to hear about your routine use of the -race flag 👍 And yes, I support running the TCK with the flag enabled to get covered in this area of Go code, provided that it does not create any (infrastructural) burden.
| cart := domain.Cart{ | ||
| Items: make([]*domain.LineItem, 0), | ||
| } | ||
| cart.Items = append(cart.Items, sc.cart...) |
There was a problem hiding this comment.
Does this need to be variadic instead of being implicitly handled by the Go runtime?
There was a problem hiding this comment.
how it can be "implicitly handled by the Go runtime"? – I think thats not possible.
func append(slice []Type, elems ...Type) []Type is variadic, but as far as I know, the type of the second argument, Type, has to be the element type of the first argument of append by the language spec. As sc.cart is of type []*domain.LineItem I have to suffix the second argument with the ellipsis ... to be spread into multiple arguments. This way, I don't have to loop over sc.cart.
There was a problem hiding this comment.
That makes sense (I was thinking out loud to eliminate a possible source of flaw, but you have satisfied my potential concern, all good here) 👍
There was a problem hiding this comment.
I was thinking out loud to eliminate a possible source of flaw
You're most welcome. Please do at any time.
@akramtexas thank you very much for your time you kindly took to review this PR. I'll have time tomorrow evening to go through your review comments and I'll look forward to any discussions that arise from. |
… a potential panic from descriptor.ForMessage
|
@marcellanz - The pleasure is all mine 👍
|
… to run go-support tests in a container, fail properly in bash scripts
| host, ok := os.LookupEnv("HOST") | ||
| if !ok { | ||
| log.Fatalf("unable to get environment variable \"HOST\"") | ||
| return fmt.Errorf("unable to get environment variable \"HOST\"") |
There was a problem hiding this comment.
Much more meaningful response here, and elsewhere in your latest update 👍
| } | ||
| marshal, err := proto.Marshal(message) | ||
| if err != nil { | ||
| log.Fatalf("unable to Marshal") | ||
| return nil, fmt.Errorf("%s, %w", err, ErrMarshal) |
There was a problem hiding this comment.
Excellent use of multi-argument logging. Succinctly done 👍
|
@marcellanz I have added a couple more comments this Sunday morning (based on your latest change-set push). Regarding my set of prior comments, I thank you for addressing them, plus I appreciated your detailed responses, including this one, which, incidentally, makes perfect sense:
|
|
@akramtexas github hides "outdated" comments. try to find "show outdated" buttons to show them. outdated means, updated by a changing commit. |
| // The gRPC implementation returns the rpc return method | ||
| // and an error as a second return value. | ||
| errReturned := called[1] | ||
| if errReturned.Interface() != nil && errReturned.Type().Name() == "error" { // FIXME: looks ugly |
There was a problem hiding this comment.
calling .Interface() would panic if it cant; better call CanInterface()
…t_tests # Conflicts: # build.sbt
|
@marcellanz Great progress here! Would you prefer to have this under its own cloudstateio/go-support repository? I'm thinking we might want to start splitting language supports into their own repos rather soon-ish, to make sure that independent progress can be made. Let me know what you think! |
@viktorklang I would prefer it yes. The go related cloudstate documentation would stay within cloudstateio/cloudstate I think and the godoc's then can be served through cloudstateio/go-support via godoc.org, the way Go likes to get api doc delivered. Then I'll have to figure how to build the TCK part, but it might be just a For any splitted language-support repo and said in #104 and like these two points above, the splitting affects documentation, tck builds and the sharing of the protocol with its *.proto files. As we moved the protobuf files out of the go-support branch to have them not duplicated, they'll have to be somewhere for an build of the language repo. I'll look into that and what the options are. |
|
@marcellanz Ok, cool. I'll check with @jroper what we need to set up. CLA validator, CI, access rights to the repo etc. |
|
@marcellanz I created this now, I'll have to configure it soon-ish: https://github.com/cloudstateio/go-support |
@viktorklang Cool. I assume this PR then is better not merged here. I'll fork it and prepare its separation from the main repo. |
|
@marcellanz Let's try that! |
…de), emit events through a subscription. Starting from here, get prepared to be in a dedicated repo for cloudstate.
|
@akramtexas as shortly discussed above I will close this PR in the coming days as I'm moving the code into a separate go-support repository. I will reference this then closed PR within a new PR. The task is captured in this issue on the new repository. Thank you for your review input so far. I'll looking forward to the initial PR on the go-support repo which will have all input of yours included; I think I will also rebase whats been done so far to have a clean history for the first commit. |
|
@marcellanz I, too, support that this PR is better off not being merged here, and instead going with the option of forking and separating (the work done here, to date) from the main repo ✅ I am happy to contribute (and will continue to do so) with review input based on my know-how of |
|
as in cloudstateio/go-support#1 announcent, I close this PR as it will be moved to its own repository. |
This PR implements Go support as a CloudState API Client Library and should fix #3 to a good amount.
Included is