feat(schema/appdata)!: efficiency & data model improvements aligned with server/v2 #21305
Conversation
WalkthroughWalkthroughThis update introduces significant enhancements across the application, notably improving state change handling, packet processing, and listener functionality. Key features include a new batching mechanism for packets, modifications to data structures for clearer representation, and improved asynchronous processing in listeners. These changes collectively aim to enhance performance, maintainability, and clarity within the codebase. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
|
||
| // Attributes lazily returns the key-value attribute representation of the event. | ||
| Attributes ToEventAttributes | ||
| } |
There was a problem hiding this comment.
I would propose we use the above Event structure in STF and app manager instead of what we have now. It can support either key-value events or JSON events lazily and tracks tx, msg and event indexes directly (rather than pushed down into string formatted attributes). This could be mirrored in core/app
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
schema/appdata/batch.go (2)
3-9: Ensure Interface Documentation Clarity.The
BatchablePacketinterface documentation is clear but could benefit from specifying whyCommitDatashould not be batched. This will help future developers understand the design decision.+// CommitData should not be batched because it forces synchronization of asynchronous listeners, which can negate the efficiency benefits of batching.
17-29: Consider Handling Listener Nil Check Outside Loop.In the
applymethod, the check forl.onBatch != nilcould be moved outside the loop for clarity and efficiency, although the current implementation is correct.func (p PacketBatch) apply(l *Listener) error { if l.onBatch != nil { return l.onBatch(p) } for _, packet := range p { if err := packet.apply(l); err != nil { return err } } return nil }schema/appdata/batch_test.go (1)
9-19: Test Coverage is Good but Consider Additional Edge Cases.The
TestBatchfunction effectively tests the batch processing. Consider adding edge cases, such as empty batches or batches with mixed valid and invalid packets, to ensure robustness.schema/decoding/resolver.go (1)
44-49: EncodeModuleName Implementation is Correct.The
EncodeModuleNamemethod correctly checks for module existence and handles errors appropriately. Consider documenting the expected format of the input string.+// EncodeModuleName expects a valid module name string that exists in the module set.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (11)
- core/store/changeset.go (1 hunks)
- schema/appdata/async.go (1 hunks)
- schema/appdata/batch.go (1 hunks)
- schema/appdata/batch_test.go (1 hunks)
- schema/appdata/data.go (3 hunks)
- schema/appdata/listener.go (1 hunks)
- schema/appdata/mux.go (1 hunks)
- schema/appdata/packet.go (1 hunks)
- schema/decoding/decoding_test.go (1 hunks)
- schema/decoding/middleware.go (3 hunks)
- schema/decoding/resolver.go (2 hunks)
Files skipped from review due to trivial changes (2)
- core/store/changeset.go
- schema/appdata/packet.go
Additional context used
Path-based instructions (9)
schema/appdata/batch.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/batch_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/decoding/resolver.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/listener.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/decoding/middleware.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/mux.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/data.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/async.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/decoding/decoding_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (17)
schema/appdata/batch.go (1)
31-41: Interface Implementation Looks Good.The implementation of
isBatchablePacketfor each data type is straightforward and correct. This ensures that only appropriate packet types are batchable.schema/appdata/batch_test.go (1)
62-80: Async Test Coverage is Adequate.The
TestBatchAsyncfunction tests asynchronous processing well. Ensure that the asynchronous behavior is reliably tested by considering potential race conditions or delays.schema/decoding/resolver.go (1)
37-42: DecodeModuleName Implementation is Correct.The
DecodeModuleNamemethod correctly checks for module existence and returns an appropriate error if not found. Ensure that all potential byte inputs are considered.schema/appdata/listener.go (2)
40-44: Expanded documentation forCommitfunction is clear and helpful.The expanded documentation provides necessary clarity on the blocking behavior of the
Commitfunction when used with asynchronous listeners. This is a valuable addition for developers to understand the implications of usingCommitin different contexts.
47-49: The addition ofonBatchfunction is a valuable enhancement.The
onBatchfunction facilitates efficient forwarding of packet batches to asynchronous listeners, aligning well with the goal of improving performance. This addition is well-structured and fits seamlessly into theListenerstruct.schema/decoding/middleware.go (3)
26-26: The introduction ofmoduleNamesmap enhances robustness.The
moduleNamesmap effectively improves the handling of module names by dynamically mapping actor identifiers to module names. This change enhances the robustness and efficiency of the middleware's operation.
38-47: Efficient handling of module names usingmoduleNamesmap.The logic for checking and decoding module names using the
moduleNamesmap is efficient. It reduces redundant decoding operations by caching resolved module names, thereby improving performance.
50-76: Streamlined module codec handling is effective.The updates to module codec handling, facilitated by the
moduleNamesmap, ensure consistent and efficient codec lookup and initialization. This contributes to the overall improvements in the middleware's performance and maintainability.schema/appdata/mux.go (1)
127-135: The addition ofonBatchfunction enhancesListenerMux.The
onBatchfunction efficiently processes packet batches for multiple listeners, improving theListenerMux's capability to manage listeners. The implementation is clean and includes appropriate error handling.schema/appdata/data.go (6)
44-48: LGTM: StructEventDataupdated to include multiple events.The addition of the
Eventsfield enhances the representation of multiple events, aligning with the PR objectives.
Line range hint
50-74: LGTM: New structEventintroduced.The
Eventstruct provides a detailed representation of events, supporting the new functionality effectively.
76-78: LGTM: Type aliasEventAttributeintroduced.The alias simplifies the representation of key-value pairs for event attributes.
86-88: LGTM: Type aliasToEventAttributesintroduced.This alias provides a clear method for retrieving event attributes.
90-91: LGTM: StructKVPairDataupdated to useActorKVPairUpdate.This change broadens the context of updates, enhancing flexibility.
94-100: LGTM: New structActorKVPairUpdateintroduced.This struct effectively represents updates associated with an actor, aligning with the new data model.
schema/appdata/async.go (1)
154-161: LGTM: SimplifiedCommitassignment and addedonBatchfunction.The changes simplify the logic and enhance functionality by supporting batch processing.
schema/decoding/decoding_test.go (1)
300-307: LGTM: UpdatedSetmethod to useActorKVPairUpdate.The change aligns with the updated data model and enhances robustness. Ensure that the tests adequately cover these changes.
To verify test coverage, check for tests that validate the behavior of
Setwith the newActorKVPairUpdatestructure.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
…ta-packet-batches
…nto aaronc/appdata-packet-batches
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- schema/appdata/async.go (1 hunks)
- schema/appdata/listener.go (1 hunks)
- schema/appdata/mux.go (1 hunks)
- schema/appdata/packet.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- schema/appdata/async.go
- schema/appdata/listener.go
- schema/appdata/mux.go
- schema/appdata/packet.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- schema/appdata/batch_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- schema/appdata/batch_test.go
Description
This PR makes the following updates:
coreStateChangesis structurally typed so we can send the same data to listeners with no copying in server/v2schema/appdata:PacketBatchfor sending a whole batch of updates to the async listeners all at onceKVPairDatais aligned withcoreStateChangesEventDatacan now take multipleEventsEvents can lazily produce key-value pairs so that this type could now be used in stf, app manager instead of what's there now for more efficiencyschema/decoding,DecoderResolverwas updated to decode[]byteactor data intostringmodule names to align with how things are done in storage and server/v2Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
EventData, improving data representation.Bug Fixes
AsyncListenerfunction, ensuring theCommitfunction is always defined.Documentation
Packetinterface and listener functionalities for clearer guidance.Tests