Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Access] Update state stream API to return json-cdc encoded events #4533

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

peterargue
Copy link
Contributor

Fix the State Stream API to return JSON-CDC encoded events like the rest of the AccessAPI.

A more robust change will be included on master that includes options to override the version for an individual AN

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #4533 (42ba03b) into v0.31 (6866c12) will increase coverage by 0.05%.
The diff coverage is 21.73%.

@@            Coverage Diff             @@
##            v0.31    #4533      +/-   ##
==========================================
+ Coverage   53.74%   53.80%   +0.05%     
==========================================
  Files         893      893              
  Lines       84533    84577      +44     
==========================================
+ Hits        45435    45506      +71     
+ Misses      35572    35527      -45     
- Partials     3526     3544      +18     
Flag Coverage Δ
unittests 53.80% <21.73%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
engine/common/rpc/convert/convert.go 30.30% <18.75%> (-0.42%) ⬇️
engine/access/state_stream/handler.go 38.70% <28.57%> (+38.70%) ⬆️

... and 9 files with indirect coverage changes

Comment on lines +731 to +733
func EventsToMessagesFromVersion(flowEvents []flow.Event, version execproto.EventEncodingVersion) ([]*entities.Event, error) {
events := make([]*entities.Event, len(flowEvents))
for i, e := range flowEvents {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you won't need this after merging this: #4522 (review), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will, but I have a followup PR that will replace this. it's just taken longer than I had originally planned so I wanted to get this out sooner

@@ -927,6 +957,21 @@ func MessageToChunk(m *entities.Chunk) (*flow.Chunk, error) {
}, nil
}

// BlockExecutionDataEventPayloadsToJson converts all event payloads from CCF to JSON-CDC in place
// This is a temporary workaround until a more robust solution is implemented
func BlockExecutionDataEventPayloadsToJson(m *entities.BlockExecutionData) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do this in-place?

Copy link
Contributor Author

@peterargue peterargue Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid copying the whole object. it's being done on the precomputed entity object so it won't affect the original objects

@peterargue peterargue requested a review from koko1123 June 30, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants