bug: Handle paginated querying to prices-keeper in slinky + pricefeed daemon #1177
Conversation
WalkthroughThe recent updates focus on optimizing the efficiency and reliability of fetching market parameters within a blockchain protocol's daemons. The changes involve transitioning to a new method of retrieving market parameters in a paginated manner to enhance data handling and reduce network resource load. These optimizations are applied consistently across various components, such as market pair fetching and price feed updates, leading to a more scalable and robust system for managing market data. 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 Configration File (
|
nivasan1
changed the title
nivasan1
force-pushed
the
nv/handle-grpc-request-pagination
branch
from
6f374ca
to
768c4bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go (2 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request_test.go (1 hunks)
Additional comments: 5
protocol/daemons/types/paginated_grpc_request.go (2)
- 14-37: The implementation of
AllPaginatedMarketParamscorrectly handles pagination for gRPC requests. However, consider adding error logging within the function for better observability and troubleshooting.- 48-63: The
HandlePaginatedQueryfunction is well-implemented for handling paginated queries. It's recommended to add comments explaining the logic, especially around the pagination continuation condition, to improve readability.protocol/daemons/slinky/client/market_pair_fetcher.go (1)
- 91-96: The update to use
AllPaginatedMarketParamsinFetchIdMappingsis correct and aligns with the PR's objective to handle large datasets efficiently. Ensure that error handling fromAllPaginatedMarketParamsis adequately addressed downstream.protocol/daemons/types/paginated_grpc_request_test.go (1)
- 20-97: The tests for
AllPaginatedMarketParamsand error handling scenarios are comprehensive and correctly validate the functionality. Consider adding a test case for scenarios where the pagination response is empty or malformed to ensure robustness.protocol/daemons/pricefeed/client/sub_task_runner.go (1)
- 352-360: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [331-357]
The update to use
AllPaginatedMarketParamsinRunMarketParamUpdaterTaskLoopis correct and improves the efficiency of market parameter updates. Ensure that the error handling and logging mechanisms are sufficient to identify issues during runtime.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go (2 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go
- protocol/daemons/slinky/client/market_pair_fetcher.go
- protocol/daemons/types/paginated_grpc_request.go
- protocol/daemons/types/paginated_grpc_request_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/daemons/slinky/client/market_pair_fetcher.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request.go (1 hunks)
- protocol/daemons/types/paginated_grpc_request_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/daemons/slinky/client/market_pair_fetcher.go
- protocol/daemons/types/paginated_grpc_request.go
- protocol/daemons/types/paginated_grpc_request_test.go
|
|
||
| // AllPaginatedMarketParams returns all MarketParams from the prices module, paginating through the results and | ||
| // returning a list of all the aggregated market-params. | ||
| func AllPaginatedMarketParams(ctx context.Context, client pricestypes.QueryClient) ([]pricestypes.MarketParam, error) { |
There was a problem hiding this comment.
nit: maybe we can move this function to daemons/pricefeed?
There was a problem hiding this comment.
This is method is used in both the pricefeed / slinky daemon. Hence I didn't want to introduce a dependency from one of those to the other. Happy to move this into pricefeed if this is not a concern
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go (3 hunks)
- protocol/daemons/shared/paginated_grpc_request.go (1 hunks)
- protocol/daemons/shared/paginated_grpc_request_test.go (1 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/daemons/pricefeed/client/sub_task_runner.go
- protocol/daemons/slinky/client/market_pair_fetcher.go
Additional comments: 4
protocol/daemons/shared/paginated_grpc_request.go (2)
- 10-11: The
PaginatedRequestLimitis set to 10,000, which might be too high for certain environments or could lead to memory issues if not handled properly. Consider making this configurable or validating this choice with load testing to ensure it's a safe default for your system's expected load and infrastructure capabilities.- 14-37: The
AllPaginatedMarketParamsfunction correctly implements paginated querying to fetch all market parameters. However, it's important to ensure that theclient.AllMarketParamscall does not have any side effects or limitations on the number of items it can fetch in a single call that could affect the pagination logic. Additionally, consider adding error handling for potential issues with the network or the gRPC service itself, ensuring robustness in real-world scenarios.protocol/daemons/shared/paginated_grpc_request_test.go (2)
- 20-97: The test
TestPaginatedGRPCRequestseffectively covers the scenario of successfully fetching paginated market parameters. It's well-structured and mocks the expected behavior of the gRPC client. However, consider adding assertions to verify that the pagination requests are made with the correctNextKeyvalues and that the limit is respected in each request. This would ensure the pagination logic is correctly implemented and adheres to the expected behavior.- 100-143: The test
TestPaginatedGRPCRequestsWithErrorcorrectly tests the error handling in the paginated request process. It's important to also test scenarios where errors occur after the first page has been successfully fetched, to ensure the function behaves correctly in partially successful scenarios. This would provide a more comprehensive test coverage for the error handling logic.
| func HandlePaginatedQuery(ctx context.Context, pq PaginatedQuery, initialPagination *query.PageRequest) error { | ||
| for { | ||
| // make the query | ||
| resp, err := pq(ctx, initialPagination) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // break if there is no next-key | ||
| if resp.GetPagination() == nil || len(resp.GetPagination().NextKey) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // otherwise, update the next-key and continue | ||
| initialPagination.Key = resp.GetPagination().NextKey | ||
| } | ||
| } |
There was a problem hiding this comment.
The HandlePaginatedQuery function effectively implements the pagination logic. However, it's crucial to consider the potential for infinite loops if the service continuously returns a NextKey. Implementing a safeguard, such as a maximum number of iterations or a timeout, could prevent such scenarios, ensuring the function doesn't hang indefinitely under certain conditions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/slinky/client/market_pair_fetcher.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/daemons/slinky/client/market_pair_fetcher.go
nivasan1
force-pushed
the
nv/handle-grpc-request-pagination
branch
from
6dbf4eb
to
bc4720a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go (3 hunks)
- protocol/daemons/shared/paginated_grpc_request.go (1 hunks)
- protocol/daemons/shared/paginated_grpc_request_test.go (1 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go
- protocol/daemons/shared/paginated_grpc_request.go
- protocol/daemons/shared/paginated_grpc_request_test.go
- protocol/daemons/slinky/client/market_pair_fetcher.go
nivasan1
force-pushed
the
nv/handle-grpc-request-pagination
branch
from
bc4720a
to
e424602
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go (3 hunks)
- protocol/daemons/shared/paginated_grpc_request.go (1 hunks)
- protocol/daemons/shared/paginated_grpc_request_test.go (1 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/daemons/pricefeed/client/sub_task_runner.go
- protocol/daemons/shared/paginated_grpc_request.go
- protocol/daemons/shared/paginated_grpc_request_test.go
- protocol/daemons/slinky/client/market_pair_fetcher.go
Changelist
[Describe or list the changes made in this PR]
Test Plan
Unit testing + changes loadtested using petri
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.