Full Node Streaming Recurring snapshots #2079
Conversation
WalkthroughThis update enhances the GitHub Actions workflows by adding triggers for branches prefixed with Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant AWS ECR
participant Streaming Manager
Developer->>GitHub Actions: Push to jonfung/** branch
GitHub Actions->>AWS ECR: Build & Push Image
GitHub Actions->>Streaming Manager: Initialize Streaming with new interval
Streaming Manager-->>Streaming Manager: Set snapshotBlockInterval
Streaming Manager->>OrderbookSubscription: Emit Snapshots
Poem
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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/protocol-build-and-push-snapshot.yml (1 hunks)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
Additional comments not posted (11)
.github/workflows/protocol-build-and-push.yml (1)
6-6: Branch patternjonfung/**added to workflow triggers.This addition allows the workflow to trigger on branches prefixed with
jonfung/, enhancing flexibility for development and testing..github/workflows/protocol-build-and-push-snapshot.yml (1)
6-6: Branch patternjonfung/**added to workflow triggers.This addition allows the workflow to trigger on branches prefixed with
jonfung/, enhancing flexibility for development and testing.protocol/streaming/full_node_streaming_manager.go (8)
51-51: New fieldsnapshotBlockIntervaladded toFullNodeStreamingManagerImpl.This field allows configuration of block intervals for emitting snapshots, improving control over data streaming.
59-59: New fieldinitializeadded toOrderbookSubscription.The
sync.Onceinstance ensures that initialization logic runs only once, unless reset, enhancing control over subscription initialization.
75-75: New fieldsnapshotDelayBlockadded toOrderbookSubscription.This field tracks the block height for initial snapshots, adjusted by
snapshotBlockInterval, enabling precise snapshot timing.
83-83: Updated constructorNewFullNodeStreamingManagerto acceptsnapshotBlockInterval.This change ensures that the streaming manager can be initialized with the new snapshot interval functionality.
100-100: Initialization ofsnapshotBlockIntervalinNewFullNodeStreamingManager.The constructor correctly initializes the new field, ensuring the snapshot interval is set upon manager creation.
163-163: Use ofsync.OnceinSubscribemethod.The
sync.Onceinstance is initialized for each subscription, ensuring that initialization logic is executed only once per subscription.
691-697: Resettingsync.Oncebased onsnapshotBlockInterval.This logic ensures that snapshots are re-sent at specified intervals, enhancing the responsiveness of orderbook updates.
714-716: SettingsnapshotDelayBlockafter sending snapshots.This logic updates the
snapshotDelayBlockto track the block height modulo the snapshot interval, maintaining correct snapshot timing.protocol/app/app.go (1)
2033-2034: Consider making the hardcoded value configurable.The hardcoded value
50should be replaced with a configurable option to enhance flexibility and maintainability. The TODO comment already suggests this improvement.- 50, + appFlags.YourConfigurableOption,Ensure that the configuration option is properly defined and documented.
jonfung-dydx
force-pushed
the
jonfung/fns_recur_snapshot
branch
from
6a39c22
to
cbc94e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/protocol-build-and-push-snapshot.yml (1 hunks)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/protocol-build-and-push-snapshot.yml
- .github/workflows/protocol-build-and-push.yml
- protocol/app/app.go
- protocol/streaming/full_node_streaming_manager.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
protocol/app/flags/flags.go (3)
28-28: Add documentation for the new field.Consider adding a comment to explain the purpose of
FullNodeStreamingSnapshotIntervalin theFlagsstruct for better maintainability.// Interval for emitting snapshots in full node streaming. FullNodeStreamingSnapshotInterval uint32
51-51: Ensure consistency in naming conventions.The constant
FullNodeStreamingSnapshotIntervalshould follow the naming pattern of other constants, such as prefixing withFlagfor clarity.- FullNodeStreamingSnapshotInterval = "fns-snapshot-interval" + FlagFullNodeStreamingSnapshotInterval = "fns-snapshot-interval"
71-71: Clarify the default value's purpose.Consider adding a comment to explain why the default value for
FullNodeStreamingSnapshotIntervalis0, indicating the behavior of sending one initial snapshot.// Default value for snapshot interval, 0 means one initial snapshot. DefaultFullNodeStreamingSnapshotInterval = 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/app/app.go (1 hunks)
- protocol/app/flags/flags.go (7 hunks)
- protocol/app/flags/flags_test.go (8 hunks)
- protocol/streaming/full_node_streaming_manager.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/app.go
- protocol/streaming/full_node_streaming_manager.go
Additional comments not posted (10)
protocol/app/flags/flags.go (3)
123-128: LGTM! Ensure correct flag registration.The registration of
FullNodeStreamingSnapshotIntervalinAddFlagsToCmdlooks good. Ensure that the flag is correctly parsed and utilized elsewhere in the application.
167-169: Ensure error message clarity.The error message in the validation logic is clear and informative, ensuring that users understand the constraint on
FullNodeStreamingSnapshotInterval.
194-194: Verify retrieval logic.The retrieval of
FullNodeStreamingSnapshotIntervalinGetFlagValuesFromOptionsis consistent with other flags. Ensure that this value is correctly used in the application logic.Verification successful
Retrieval and Usage of
FullNodeStreamingSnapshotIntervalVerifiedThe
FullNodeStreamingSnapshotIntervalis consistently retrieved and used across the application logic. It is checked and utilized inapp.go, and its behavior is validated through tests inflags_test.go. No issues found with its integration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `FullNodeStreamingSnapshotInterval` in the application logic. # Test: Search for the usage of `FullNodeStreamingSnapshotInterval`. Expect: Correct usage in logic handling. rg --type go $'FullNodeStreamingSnapshotInterval'Length of output: 1933
protocol/app/flags/flags_test.go (7)
41-43: LGTM! Test coverage for flag registration.The addition of
FullNodeStreamingSnapshotIntervaltoTestAddFlagsToCommandensures that the flag is correctly registered. This test coverage is appropriate.
66-72: LGTM! Default values in validation tests.The inclusion of
FullNodeStreamingSnapshotIntervalin the default values test case ensures that the default behavior is correctly validated.
152-163: LGTM! Validation error test case.The test case for invalid
FullNodeStreamingSnapshotIntervalvalues correctly checks the enforcement of the minimum threshold, ensuring robustness.
164-174: LGTM! Validation success test case.The test case for a valid
FullNodeStreamingSnapshotIntervalvalue of 50 ensures that the application logic correctly handles valid configurations.
Line range hint
203-216:
LGTM! Retrieval test for default values.The test case
Sets to default if unsetcorrectly verifies thatFullNodeStreamingSnapshotIntervaldefaults to0when not set, ensuring expected behavior.
Line range hint
230-242:
LGTM! Retrieval test for set values.The test case
Sets values from optionsverifies thatFullNodeStreamingSnapshotIntervalis correctly retrieved from options, ensuring accurate configuration handling.
296-300: LGTM! Retrieval assertion for new flag.The assertion for
FullNodeStreamingSnapshotIntervalinTestGetFlagValuesFromOptionsensures that the retrieved value matches the expected configuration.
Summary by CodeRabbit
New Features
jonfung/, enhancing deployment flexibility.Bug Fixes
Documentation