Go support for CloudState User Functions #5

marcellanz · 2019-10-11T23:57:51Z

This PR implements Go support as a CloudState API Client Library and should fix cloudstateio/cloudstate#3 to a good amount.

Included is

The implementation of the Discovery and EventSourced Services of CloudState.
The implementation of the CloudState Serialization Convention.
The implementation of the TCK Shopping Cart example, used to be verified by the TCK.
Build support for travis providing a docker image so that the CloudState TCK can be run without the requirement to build the Go Shopping Cart by itself. Successful run here.
Paradox Documentation is provided separately in the main repository; see PR#128.

Before this separate repository was created, a previous Draft-PR was opened and reviewed partly.

https://golang.org/doc/faq#go_or_golang)

# Conflicts: # README.md

https://golang.org/doc/faq#go_or_golang)

…ee/feature/go-support on c94dd10

- handleCommand cleanup - added codecov to travis build - refined snapshot handling - snapshot support without tests - implemented protobuf any serialization by CloudState conventions - added benchmarks for CloudState serialization - cleaned up documentation - travis build for other OS (win,linux, macOS) - refactored EventSourcedHandler.handleCommand - aligned and cleaned up failure handling

# Conflicts: # README.md

…t on first implementation.

codecov-io · 2019-10-12T00:00:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@081e913). Click here to learn what that means.
The diff coverage is 51.78%.

@@            Coverage Diff            @@
##             master       #5   +/-   ##
=========================================
  Coverage          ?   51.78%           
=========================================
  Files             ?        9           
  Lines             ?      645           
  Branches          ?        0           
=========================================
  Hits              ?      334           
  Misses            ?      255           
  Partials          ?       56

Impacted Files	Coverage Δ
cloudstate/eventsourced_reply.go	`10.93% <10.93%> (ø)`
cloudstate/entity_key.pb.go	`100% <100%> (ø)`
cloudstate/cloudstate.go	`43.69% <43.69%> (ø)`
cloudstate/protosupport.go	`50% <50%> (ø)`
cloudstate/eventsourced.go	`52.04% <52.04%> (ø)`
cloudstate/encoding/any_json.go	`62.96% <62.96%> (ø)`
cloudstate/encoding/any_primitive.go	`68.57% <68.57%> (ø)`
cloudstate/marshal_proto.go	`71.42% <71.42%> (ø)`
cloudstate/event.go	`88% <88%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 081e913...accb0f3. Read the comment docs.

jsravn

First review pass :). There's a lot here so will take another pass tomorrow. In addition to my comments I had a few other general thoughts:

It would be nice if we can run the TCK in this repo so we get immediate feedback from changes whether we broke anything. But not sure how feasible that is, at least to do now.
Need to think how we handle the go "logging problem" where we need to give some level of control to the user over logging.
I think the API needs some improvements which is where my first pass was focused on but I didn't quite get to everything.

Bear with me as I'm also fairly new to cloudstate, so I'm learning the protocol as well as I go.

.travis.yml

build/fetch-cloudstate-pb.sh

.travis.yml

build/TCK.Dockerfile

cloudstate/cloudstate.go

jsravn · 2019-10-14T15:04:09Z

cloudstate/cloudstate.go

+		persistenceID = e.PersistenceID
+	}
+	r.entitySpec.Entities = append(r.entitySpec.Entities, &protocol.Entity{
+		EntityType:    EventSourced,


Is there any way to get the value of EventSourced from the service descriptor?

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

var _EventSourced_serviceDesc = grpc.ServiceDesc{ ServiceName: "cloudstate.eventsourced.EventSourced", HandlerType: (*EventSourcedServer)(nil), Methods: []grpc.MethodDesc{}, Streams: []grpc.StreamDesc{ { StreamName: "handle", Handler: _EventSourced_Handle_Handler, ServerStreams: true, ClientStreams: true, }, }, Metadata: "cloudstate/event_sourced.proto", }

@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.

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:

func (r *EntityDiscoveryService) registerEntity(e *EventSourcedEntity, config DescriptorConfig) error { if err := r.resolveFileDescriptors(config); err != nil { return fmt.Errorf("failed to resolveFileDescriptor for DescriptorConfig: %+v: %w", config, err) } persistenceID := e.entityName if e.PersistenceID != "" { persistenceID = e.PersistenceID } fd, err := desc.LoadFileDescriptor("cloudstate/event_sourced.proto") if err != nil { return err } r.entitySpec.Entities = append(r.entitySpec.Entities, &protocol.Entity{ EntityType: fd.FindService("EventSourced").GetFullyQualifiedName(), ServiceName: e.ServiceName, PersistenceId: persistenceID, }) return r.updateSpec() }

cloudstate/eventsourced.go

jroper · 2019-10-14T20:57:06Z

Sorry about the close/reopen noise, was just testing the CLA checker integration.

jroper · 2019-10-14T21:48:26Z

@marcellanz does it make sense to enable codecov for this repo? I've never enabled it before, if you could point me to instructions that would be helpful.

marcellanz · 2019-10-14T22:34:44Z

@jroper I haven't used it before and configured the simplest way I found through their bash uploader https://docs.codecov.io/v4.3.0/docs/about-the-codecov-bash-uploader

which does not need a token configured because of the public repo. I simply used their proposed integration
https://github.com/codecov/example-go and it seems to comment pull request which I didn't knew before. This can be configured by a *.yml file, but I haven't done that yet.

I added the badge to the README.md and my intention was to have just this badge together with two others there (godoc, travis build). I think the comment on the pull request is not needed, but having test coverage stats on a PR seems to be nice to have, even if its coverage at the moment is not that great.

So in this regard, I think you don't have to configure anything for the repo.

jroper · 2019-10-14T23:11:48Z

No worries - note though that we could have everything more automatic, with webhooks and commit statuses, if desired.

marcellanz · 2019-10-15T07:24:09Z

First review pass :). There's a lot here so will take another pass tomorrow.

@jsravn Thanks a lot James for this thorough review of the PR, I appreciate it lot. I'm away this week but I'm on the contributors call today if we might want to discuss certain issues. I think especially of the "logging problem" where we could define how to do it for language support libraries in general. I'm looking forward to your 2nd pass :-)

marcellanz · 2019-10-15T07:30:19Z

No worries - note though that we could have everything more automatic, with webhooks and commit statuses, if desired.

@jroper all right, let me know if we should have a closer look to codecov where it helps and how to integrate it best.

build/build-and-publish-docker-image-tck.sh

…eio#5

jsravn · 2019-10-16T10:25:28Z

@jsravn Thanks a lot James for this thorough review of the PR, I appreciate it lot. I'm away this week but I'm on the contributors call today if we might want to discuss certain issues. I think especially of the "logging problem" where we could define how to do it for language support libraries in general. I'm looking forward to your 2nd pass :-)

Hi @marcellanz, no problem :). I'd like to get this PR merged soon so we can collaborate more easily. The logging can probably be done in another PR - this PR doesn't need to be perfect. I was unable to make the call yesterday, but I will try to make next week.

marcellanz · 2019-10-16T12:33:52Z

@jsravn Thanks a lot James for this thorough review of the PR, I appreciate it lot. I'm away this week but I'm on the contributors call today if we might want to discuss certain issues. I think especially of the "logging problem" where we could define how to do it for language support libraries in general. I'm looking forward to your 2nd pass :-)

Hi @marcellanz, no problem :). I'd like to get this PR merged soon so we can collaborate more easily. The logging can probably be done in another PR - this PR doesn't need to be perfect. I was unable to make the call yesterday, but I will try to make next week.

@jsravn Great, to get up-to-date with the calls, there are minutes available too https://docs.google.com/document/d/18_gS04R2ELAVmatW2nN5iR5PRmYqzQDbtQeS41oOIys
where latest stuff and next actions are summarized.

jsravn

Okay, I finished my second pass and so I think I'm mostly done w/ my review.

I think my main concern on this pass is the implicitness of the API. There is a lot of runtime stuff going on (reflection finding of methods by convention, runtime casts to interfaces), that doesn't feel like idiomatic go to me. I think we should err on the side of explicitness - right now it's not very obvious "what to implement" on the event sourced entity.

I'd like to see a single interface with the required handler methods on it that the EventSourcedEntity needs to implement (including handleCommand, handleEvents, handleSnapshot). Then it becomes clear to a user what they should implement and the control flow is more obvious and explicit. It will also be a compile error rather than runtime if something is missing. I also think we should really avoid finding methods by convention, matching against their types with reflection - I haven't seen that kind of approach in a mainstream golang library, and I think it may dissuade users from using it.

cloudstate/eventsourced.go

jsravn · 2019-10-16T13:29:23Z

cloudstate/eventsourced.go

+	for {
+		if failed != nil {
+			return failed
+		}


How come we close the stream if the user function returned an error? Shouldn't we just return the protocol.Failure and keep processing the stream?

I think this here is ok; handleFailure will handle every error out of any Command, Event or Init msg and itself sends a protocol.Failure and if sending this failure does not work, we close the stream.

But: I see now handleFailure lets the failure through if it does not match. Will fix this.

Edit: while thinking more about this I'm not sure if an error (failure) that is not a cloudState.ErrFailure or cloudstate.ErrClientActionFailure as in handleFailure checked indeed should close the stream as it is an error we would not expect.

cloudstate/eventsourced.go

tck/cmd/tck_shoppingcart/shoppingcart.go

jsravn · 2019-10-16T14:49:05Z

cloudstate/event.go

+
+type SnapshotHandler interface {
+	HandleSnapshot(snapshot interface{}) (handled bool, err error)
+}


I wonder how we can make this a bit more explicit. In other words there is no compilation error if a user doesn't implement these functions - only at runtime. Maybe we should require the event sourced entity to implement them all?

cloudstate/eventsourced.go

marcellanz · 2019-10-18T12:59:15Z

Great feedback and input on this PR. I'm still on some days off but will have a look soon on it.

viktorklang · 2019-10-21T17:38:11Z

@marcellanz Is this PR ready to merge? /cc @jroper @jsravn

marcellanz · 2019-10-21T18:37:01Z

@marcellanz Is this PR ready to merge? /cc @jroper @jsravn

@viktorklang I'd say we can merge it tomorrow, perhaps after the contributors call. James had some great input and I made some changes accordingly. I'll push that tomorrow evening.

viktorklang · 2019-10-21T19:42:34Z

@marcellanz Awesome, looking forward to the call!

…teio#5

viktorklang · 2019-10-22T19:07:38Z

Ship it!

* [go-support] as discussed in weekly call 2019-10-08: go_packages added to the main repo. * [go-support] paradox documentation for go-support. * [go-support] enable the TCK to run the shopping cart example for Go. * [PR Review Feedback] first batch of changes after review of cloudstateio/go-support#5 * [go-support] Update doc and config to Cloudstate instead of CloudState * [PR-review] fixes and issues addressed from the review feedback as well as API changes reflected from the latest updates to the go-support repo. * [PR-review] cross-PR review changes from go-support PR #10

marcellanz added 9 commits September 27, 2019 15:59

[doc] Go is called Go, GoLang is used where the term "go" is ambiguous (

52dd4dd

https://golang.org/doc/faq#go_or_golang)

Merge remote-tracking branch 'upstream/master'

ea5217b

# Conflicts: # README.md

[doc] Go is called Go, GoLang is used where the term "go" is ambiguous (

a69533c

https://golang.org/doc/faq#go_or_golang)

[initial] moved over from https://github.com/marcellanz/cloudstate/tr…

cb84071

…ee/feature/go-support on c94dd10

cleanups and added proto to the repo

83b5294

[tests] test for snapshot

5365861

Merge remote-tracking branch 'origin/master'

a84b2ee

# Conflicts: # README.md

[go-support] added missing proto files. removed old json encoding lef…

a8fdc37

…t on first implementation.

marcellanz mentioned this pull request Oct 11, 2019

feature/go_support – paradox documentation and TCK cloudstateio/cloudstate#128

Merged

Merge branch 'feature/go-support'

d234659

jsravn reviewed Oct 14, 2019

View reviewed changes

jroper closed this Oct 14, 2019

jroper reopened this Oct 14, 2019

[PR-review] added missing EOL. Removed commented code.

801f67c

marcellanz commented Oct 15, 2019

View reviewed changes

build/build-and-publish-docker-image-tck.sh Outdated Show resolved Hide resolved

[PR Review Feedback] first batch of changes after review of cloudstat…

14373da

…eio#5

jsravn reviewed Oct 16, 2019

View reviewed changes

[PR Review Feedback] second batch of changes after review of cloudsta…

accb0f3

…teio#5

viktorklang merged commit b89ecf5 into cloudstateio:master Oct 22, 2019

This was referenced Oct 22, 2019

remove handling by reflection, introduce handleCommand instead #6

Closed

Provide a Logging story and implementation #13

Open

marcellanz mentioned this pull request Oct 29, 2019

Discussion: Logging in Language Support Libraries cloudstateio/cloudstate#130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go support for CloudState User Functions #5

Go support for CloudState User Functions #5

marcellanz commented Oct 11, 2019 •

edited

codecov-io commented Oct 12, 2019 •

edited

jsravn left a comment

jsravn Oct 14, 2019

marcellanz Oct 15, 2019

jsravn Oct 16, 2019

jroper commented Oct 14, 2019

jroper commented Oct 14, 2019

marcellanz commented Oct 14, 2019 •

edited

jroper commented Oct 14, 2019

marcellanz commented Oct 15, 2019

marcellanz commented Oct 15, 2019

jsravn commented Oct 16, 2019

marcellanz commented Oct 16, 2019

jsravn left a comment

jsravn Oct 16, 2019

marcellanz Oct 22, 2019 •

edited

jsravn Oct 16, 2019

marcellanz commented Oct 18, 2019

viktorklang commented Oct 21, 2019

marcellanz commented Oct 21, 2019

viktorklang commented Oct 21, 2019

viktorklang commented Oct 22, 2019

Go support for CloudState User Functions #5

Go support for CloudState User Functions #5

Conversation

marcellanz commented Oct 11, 2019 • edited

codecov-io commented Oct 12, 2019 • edited

Codecov Report

jsravn left a comment

Choose a reason for hiding this comment

jsravn Oct 14, 2019

Choose a reason for hiding this comment

marcellanz Oct 15, 2019

Choose a reason for hiding this comment

jsravn Oct 16, 2019

Choose a reason for hiding this comment

jroper commented Oct 14, 2019

jroper commented Oct 14, 2019

marcellanz commented Oct 14, 2019 • edited

jroper commented Oct 14, 2019

marcellanz commented Oct 15, 2019

marcellanz commented Oct 15, 2019

jsravn commented Oct 16, 2019

marcellanz commented Oct 16, 2019

jsravn left a comment

Choose a reason for hiding this comment

jsravn Oct 16, 2019

Choose a reason for hiding this comment

marcellanz Oct 22, 2019 • edited

Choose a reason for hiding this comment

jsravn Oct 16, 2019

Choose a reason for hiding this comment

marcellanz commented Oct 18, 2019

viktorklang commented Oct 21, 2019

marcellanz commented Oct 21, 2019

viktorklang commented Oct 21, 2019

viktorklang commented Oct 22, 2019

marcellanz commented Oct 11, 2019 •

edited

codecov-io commented Oct 12, 2019 •

edited

marcellanz commented Oct 14, 2019 •

edited

marcellanz Oct 22, 2019 •

edited