Skip to content
This repository has been archived by the owner on Mar 11, 2022. It is now read-only.

Update to latest TCK #67

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Update to latest TCK #67

merged 1 commit into from
Dec 18, 2020

Conversation

pvlugter
Copy link
Member

Update to the latest TCK from cloudstateio/cloudstate#500

I've already published a new cloudstate-go-tck docker image with these changes, and which should work for both the current latest TCK as well as the updated TCK in cloudstateio/cloudstate#500

New TCK uncovered some small bugs:

  • Value and event sourced entities were not resetting all parts of the context after command processing, so if there's a forward or side effect then it would keep returning these for subsequent commands as well.
  • CRDT entities did not specify the persistenceId, which is currently used in the proxy as a prefix for CRDT keys. The proxy could use the serviceName directly itself, but it currently uses the persistenceId so I've updated here to pass the full service name as the persistence id for CRDT entities. This was uncovered when forwarding to the second entity type but with the same entity key.

The passivation timeout is still to be supported, and those are the only pending tests now. I've added commented out service descriptions for these to the updated protobufs (commented out because the TCK checks that there are the same number of entities and gRPC services on entity discovery).

@codecov-io
Copy link

Codecov Report

Merging #67 (b159b91) into master (03a0797) will decrease coverage by 14.42%.
The diff coverage is 4.27%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #67       +/-   ##
===========================================
- Coverage   64.21%   49.79%   -14.43%     
===========================================
  Files          32       36        +4     
  Lines        1280     1677      +397     
===========================================
+ Hits          822      835       +13     
- Misses        342      724      +382     
- Partials      116      118        +2     
Impacted Files Coverage Δ
cloudstate/action/context.go 0.00% <0.00%> (ø)
cloudstate/crdt/cmdcontext.go 47.43% <0.00%> (-1.25%) ⬇️
cloudstate/value/context.go 0.00% <0.00%> (ø)
cloudstate/action/server.go 1.16% <1.16%> (ø)
cloudstate/value/server.go 3.33% <3.33%> (ø)
cloudstate/discovery/server.go 57.89% <23.80%> (-14.11%) ⬇️
cloudstate/cloudstate.go 36.17% <28.57%> (-3.23%) ⬇️
cloudstate/eventsourced/context.go 61.53% <50.00%> (-3.47%) ⬇️
cloudstate/eventsourced/server.go 60.22% <100.00%> (+0.45%) ⬆️
... and 3 more

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 11a84a3...b159b91. Read the comment docs.

@pvlugter
Copy link
Member Author

Note: didn't remove shopping cart from the TCK, so that it still works with the currently published latest TCK, and because it looks like it's included in the docs.

@marcellanz
Copy link
Contributor

Cool! Thanks @pvlugter for the changes and fixes.

For the passivation timeout I have a branch ready for that I wanted to until the reorganisation and passivation stable.

@marcellanz
Copy link
Contributor

Looks good to me

@marcellanz marcellanz merged commit 005c956 into cloudstateio:master Dec 18, 2020
@pvlugter pvlugter deleted the tck branch December 18, 2020 22:55
@marcellanz
Copy link
Contributor

CRDT entities did not specify the persistenceId, which is currently used in the proxy as a prefix for CRDT keys. The proxy could use the serviceName directly itself, but it currently uses the persistenceId so I've updated here to pass the full service name as the persistence id for CRDT entities

@pvlugter Is setting the persistenceId temporarily or do we mandate that to be set implicit or by the user function on registration explicit in the future? We might have to document it too.
Beside that do we have to set the presistenceId once CRDTs can be persisted at all?

@pvlugter
Copy link
Member Author

CRDT entities did not specify the persistenceId, which is currently used in the proxy as a prefix for CRDT keys. The proxy could use the serviceName directly itself, but it currently uses the persistenceId so I've updated here to pass the full service name as the persistence id for CRDT entities

@pvlugter Is setting the persistenceId temporarily or do we mandate that to be set implicit or by the user function on registration explicit in the future? We might have to document it too.
Beside that do we have to set the presistenceId once CRDTs can be persisted at all?

I think this should be temporary, and the proxy updated to use the service name automatically for the key prefix. I'm not actually sure why persistence ids are required from the language support for any of the entities — seems that the service name could always be used, including for event sourced entities. And that it would be good to also support and include an entity version or similar.

@marcellanz marcellanz mentioned this pull request Jan 11, 2021
2 tasks
marcellanz added a commit to marcellanz/go-support that referenced this pull request Jan 12, 2021
marcellanz added a commit that referenced this pull request Jan 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants