(OraklNode) Update redis publish key and related codes #2222
Conversation
WalkthroughWalkthroughThe modifications primarily involve altering function signatures and variable types across multiple files, shifting from integer-based identifiers to string-based names for configuration management. This change impacts how data is processed, published, and tested within the application, ensuring a more direct and descriptive approach to handling configurations and submission data. Changes
Sequence Diagram(s)sequenceDiagram
participant Aggregator
participant Publisher
participant Database
Aggregator->>Publisher: PublishGlobalAggregateAndProof(name, globalAggregate, proof)
Publisher->>Database: db.Publish(name, globalAggregate, proof)
Database-->>Publisher: Acknowledgment
Publisher-->>Aggregator: Result
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 (13)
- node/pkg/aggregator/aggregator.go (1 hunks)
- node/pkg/aggregator/aggregator_test.go (1 hunks)
- node/pkg/aggregator/app.go (1 hunks)
- node/pkg/aggregator/globalaggregatebulkwriter.go (5 hunks)
- node/pkg/aggregator/globalaggregatebulkwriter_test.go (5 hunks)
- node/pkg/aggregator/utils.go (1 hunks)
- node/pkg/common/keys/keys.go (1 hunks)
- node/pkg/dal/collector/collector.go (4 hunks)
- node/pkg/dal/hub/hub.go (1 hunks)
- node/pkg/dal/tests/api_test.go (6 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/reporter/app_test.go (1 hunks)
Additional comments not posted (30)
node/pkg/common/keys/keys.go (1)
3-4: LGTM! Simplified function signature.The change to use a
stringparameter directly improves clarity and aligns with the PR objective of using symbol names for Redis keys.node/pkg/aggregator/utils.go (1)
21-29: LGTM! Enhanced flexibility with the new parameter.The addition of the
nameparameter aligns with the PR objective and enhances the flexibility of the function.Run the following script to verify the function usage:
Verification successful
Function usage verified successfully.
All calls to
PublishGlobalAggregateAndProofin the codebase are consistent with the new signature, including thenameparameter. No discrepancies were found.
node/pkg/aggregator/aggregator.gonode/pkg/aggregator/aggregator_test.gonode/pkg/aggregator/globalaggregatebulkwriter_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `PublishGlobalAggregateAndProof` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'PublishGlobalAggregateAndProof'Length of output: 2323
node/pkg/reporter/app_test.go (1)
64-64: LGTM! Simplified test with a consistent key.Using a hardcoded string for the Redis key in the test ensures consistent behavior and simplifies the test logic.
node/pkg/dal/tests/collector_test.go (1)
88-88: Ensure consistency with the new parameter.The addition of the
"test-aggregate"string parameter in thetestPublishDatafunction call aligns with the shift from configuration IDs to symbol names. Verify that all related tests and function calls are updated consistently.node/pkg/aggregator/globalaggregatebulkwriter_test.go (5)
25-25: Update to use configuration names.The use of
WithConfigNamesinstead ofWithConfigIdsaligns with the updated approach to use configuration names. This change enhances clarity and consistency with the new design paradigm.
43-43: Update to use configuration names.The change to
WithConfigNamesin theTestGlobalAggregateBulkWriterStartfunction is consistent with the new design approach. Ensure all related tests are updated similarly.
62-62: Update to use configuration names.The change to
WithConfigNamesin theTestGlobalAggregateBulkWriterStopfunction is consistent with the new design approach. Ensure all related tests are updated similarly.
83-83: Update to use configuration names.The change to
WithConfigNamesin theTestGlobalAggregateBulkWriterDataStorefunction is consistent with the new design approach. Ensure all related tests are updated similarly.
112-112: Verify the static string usage.The use of the static string
"test_pair"in thePublishGlobalAggregateAndProoffunction call may affect the logic of the test. Ensure this change aligns with the intended functionality and that all related tests are updated consistently.node/pkg/dal/tests/main_test.go (1)
39-40: Update function signature to use symbol names.The addition of the
nameparameter in thetestPublishDatafunction aligns with the shift from configuration IDs to symbol names. Ensure all calls to this function are updated accordingly.node/pkg/aggregator/aggregator_test.go (2)
124-124: LGTM! But verify the subscription key change.The change from
node.IDtonode.Namefor the subscription key is consistent with the PR objectives.Ensure that this change is reflected wherever
SubmissionDataStreamKeyis used.
129-129: LGTM! But verify the function signature change.The addition of
"test_pair"to thePublishGlobalAggregateAndProoffunction call is consistent with the updated function signature.Ensure that all calls to
PublishGlobalAggregateAndProofin the codebase match the new signature.Run the following script to verify the function signature change:
Verification successful
Function Signature Change Verified
All calls to the
PublishGlobalAggregateAndProoffunction in the codebase match the updated signature, including thenameparameter. No discrepancies were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `PublishGlobalAggregateAndProof` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'PublishGlobalAggregateAndProof'Length of output: 2323
node/pkg/dal/hub/hub.go (1)
142-142: LGTM! Simplified broadcast channel initialization.The use of
symboldirectly as the key ininitializeBroadcastChannelssimplifies the logic and improves performance.The removal of
configIdToSymbolis appropriate since its functionality is no longer needed.node/pkg/aggregator/globalaggregatebulkwriter.go (5)
17-17: LGTM! UpdatedReceiveChannelsto use string keys.The change from
int32tostringforReceiveChannelskeys enhances clarity and aligns with the PR objectives.
33-33: LGTM! UpdatedConfigNamesin configuration.The change from
ConfigIdstoConfigNamesreflects a broader semantic shift and aligns with the PR objectives.
50-52: LGTM! RenamedWithConfigIdstoWithConfigNames.The renaming and parameter change in
WithConfigNamesalign with the updated configuration structure.
66-73: LGTM! UpdatedNewGlobalAggregateBulkWriterto use string keys.The initialization of
ReceiveChannelswith string keys aligns with the updated configuration structure.
105-119: LGTM! UpdatedreceiveEachmethod to use string keys.The use of string keys in the
receiveEachmethod aligns with the updated configuration structure.node/pkg/dal/collector/collector.go (4)
34-34: LGTM: Change toOutgoingStreamkey type.The change from
int32tostringkeys inOutgoingStreamaligns with the shift to using symbol names, improving clarity and consistency.
Line range hint
95-110: LGTM: Initialization changes inNewCollector.The initialization of
OutgoingStreamwith string keys and the update to useconfig.NameforredisTopicsare consistent with the shift to symbol names.
231-236: LGTM: Update inprocessIncomingData.The use of
result.Symbolfor accessingOutgoingStreamis consistent with the change to string keys and ensures correct data processing.
Line range hint
279-279: Verify consistency inIncomingDataToOutgoingData.Ensure that the continued use of
int32keys forSymbolsandFeedHashesis intentional and consistent with the overall design.node/pkg/aggregator/app.go (1)
62-67: LGTM: Transition toconfigNamesinsetGlobalAggregateBulkWriter.The use of
configNamesinstead ofconfigIdsaligns with the shift to string-based configuration handling, ensuring consistency with the new data model.node/pkg/aggregator/aggregator.go (1)
285-285: LGTM: Addition ofn.NameinHandleProofMessage.Including
n.Namein thePublishGlobalAggregateAndProofcall enhances the specificity of data identification, aligning with the PR objectives.node/pkg/dal/tests/api_test.go (6)
99-99: Change to use "test-aggregate" is appropriate.The update aligns with the PR's goal to use symbol names for Redis keys, improving consistency across nodes.
171-171: Change to use "test-aggregate" is appropriate.This update is consistent with the PR's objective and aligns with the changes in other test cases.
212-212: Change to use "test-aggregate" is appropriate.This update is consistent with the PR's objective and aligns with the changes in other test cases.
260-260: Change to use "test-aggregate" is appropriate.This update is consistent with the PR's objective and aligns with the changes in other test cases.
330-330: Change to use "test-aggregate" is appropriate.This update is consistent with the PR's objective and aligns with the changes in other test cases.
476-476: Change to use "test-aggregate" is appropriate.This update is consistent with the PR's objective and aligns with the changes in other test cases.
nick-bisonai
force-pushed
the
feat/fix-invalid-subscription-issue
branch
from
bd5b825 to
d0ec476
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 (13)
- node/pkg/aggregator/aggregator.go (1 hunks)
- node/pkg/aggregator/aggregator_test.go (1 hunks)
- node/pkg/aggregator/app.go (1 hunks)
- node/pkg/aggregator/globalaggregatebulkwriter.go (5 hunks)
- node/pkg/aggregator/globalaggregatebulkwriter_test.go (5 hunks)
- node/pkg/aggregator/utils.go (1 hunks)
- node/pkg/common/keys/keys.go (1 hunks)
- node/pkg/dal/collector/collector.go (4 hunks)
- node/pkg/dal/hub/hub.go (1 hunks)
- node/pkg/dal/tests/api_test.go (6 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/reporter/app_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- node/pkg/aggregator/aggregator.go
- node/pkg/aggregator/aggregator_test.go
- node/pkg/aggregator/app.go
- node/pkg/aggregator/globalaggregatebulkwriter.go
- node/pkg/aggregator/globalaggregatebulkwriter_test.go
- node/pkg/aggregator/utils.go
- node/pkg/common/keys/keys.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/tests/collector_test.go
- node/pkg/dal/tests/main_test.go
- node/pkg/reporter/app_test.go
Additional comments not posted (2)
node/pkg/dal/hub/hub.go (1)
142-144: LGTM!The function now directly uses
symbolas the key, simplifying the logic and improving performance.The code changes are approved.
node/pkg/dal/collector/collector.go (1)
34-34: LGTM!The changes reflect a shift from using
int32identifiers to using string names, improving clarity and aligning with the new data model.The code changes are approved.
Also applies to: 95-95, 107-110, 113-118, 125-127
Description
It has been revealed that even though
DALwas subscribing to multiple nodes, there were data delay when deploying bisonai node. After investigating this issue, it has been found that there were wrong implementation regarding redis pubsubAS-IS
Publish & Subscription redis key was based on config id
TO-BE
Publish & Subscription base on symbol name
Why is it required?
Config id is not a consistent value among different nodes
Through this update it will properly utilize data from multiple nodes from DAL
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment