add inflight quote cache #2963
add inflight quote cache #2963
Conversation
|
Warning Rate limit exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 39 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes streamline the quote management system by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QuoterManager
participant InventoryManager
participant InFlightManager
User->>QuoterManager: SubmitAllQuotes()
QuoterManager->>InventoryManager: GetCommittableBalances()
InventoryManager->>InFlightManager: GetInFlightQuotes()
InFlightManager-->>InventoryManager: Return In-Flight Quotes
InventoryManager-->>QuoterManager: Return Committable Balances
QuoterManager-->>User: Submit Confirmation
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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
The pull request introduces an in-flight quote cache to optimize performance by reducing the frequency of database queries.
- New Cache Mechanism: Added
services/rfq/relayer/inventory/inflight.goto manage in-flight quotes with a caching mechanism. - Manager Integration: Modified
services/rfq/relayer/inventory/manager.goto integrate the in-flight quote cache into theinventoryManagerImplstruct. - Balance Fetching Options: Updated
services/rfq/relayer/inventory/options.goto includeShouldRefreshBalancesandSkipDBCacheoptions for more granular control. - Quote Submission Optimization: Adjusted
services/rfq/relayer/quoter/quoter.goto use the new cache mechanism in theSubmitAllQuotesmethod.
These changes aim to address performance issues by caching frequently called SQL query results, reducing latency, and optimizing resource usage.
4 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings
| return quoter | ||
| } | ||
|
|
||
| func (q *inFlightManager) GetInFlightQuotes(ctx context.Context, skipCache bool) (quotes []reldb.QuoteRequest, err error) { |
There was a problem hiding this comment.
logic: Consider adding logging for cache hits and misses to aid in debugging and performance monitoring.
| } | ||
|
|
||
| func (q *inFlightManager) GetInFlightQuotes(ctx context.Context, skipCache bool) (quotes []reldb.QuoteRequest, err error) { | ||
| if skipCache || q.shouldRefresh() { |
There was a problem hiding this comment.
logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks.
| return q.entry.quotes, nil | ||
| } | ||
|
|
||
| func (q *inFlightManager) shouldRefresh() bool { |
There was a problem hiding this comment.
style: Potential race condition if shouldRefresh is called concurrently with GetInFlightQuotes. Ensure proper locking.
| @@ -79,6 +79,8 @@ type inventoryManagerImpl struct { | |||
| meter metric.Meter | |||
| // balanceGauge is the histogram for balance | |||
| balanceGauge metric.Float64ObservableGauge | |||
| // inFlightQuoteManager is the cache for in flight quotes | |||
| inFlightQuoteManager *inFlightManager | |||
There was a problem hiding this comment.
style: Ensure cache invalidation logic is robust to prevent stale data issues.
| // get db first | ||
| // Add other committed, but incomplete statuses here | ||
| // TODO: clean me up: you can do this by having a IsLiquidityCommitted() method on the type. | ||
| inFlightQuotes, err := i.db.GetQuoteResultsByStatus(ctx, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted) | ||
| inFlightQuotes, err := i.inFlightQuoteManager.GetInFlightQuotes(ctx, reqOptions.skipDBCache) |
There was a problem hiding this comment.
style: Consider adding error handling for cache retrieval failures.
| var wg sync.WaitGroup | ||
| wg.Add(len(i.tokens)) | ||
|
|
||
| gasBalances := make(map[int]*big.Int) |
There was a problem hiding this comment.
logic: Pre-allocate gasBalances map with known chain IDs to improve performance.
| // ShouldRefreshBalances allows someone fetching balance(s) to skip the cache. | ||
| func ShouldRefreshBalances() BalanceFetchArgOption { | ||
| return func(options *balanceFetchOptions) { | ||
| options.skipCache = true | ||
| options.shouldRefreshBalances = true | ||
| } |
There was a problem hiding this comment.
style: Ensure that forcing a balance refresh does not lead to race conditions or inconsistent state.
| func SkipDBCache() BalanceFetchArgOption { | ||
| return func(options *balanceFetchOptions) { | ||
| options.skipDBCache = true |
There was a problem hiding this comment.
logic: Skipping the DB cache might lead to increased load on the database; monitor performance.
| @@ -239,7 +239,7 @@ func (m *Manager) SubmitAllQuotes(ctx context.Context) (err error) { | |||
| metrics.EndSpanWithErr(span, err) | |||
There was a problem hiding this comment.
style: Ensure inventory.SkipDBCache() does not introduce inconsistencies in balance retrieval.
| @@ -239,7 +239,7 @@ func (m *Manager) SubmitAllQuotes(ctx context.Context) (err error) { | |||
| metrics.EndSpanWithErr(span, err) | |||
| }() | |||
|
|
|||
| inv, err := m.inventoryManager.GetCommittableBalances(ctx) | |||
| inv, err := m.inventoryManager.GetCommittableBalances(ctx, inventory.SkipDBCache()) | |||
There was a problem hiding this comment.
logic: Consider the impact of skipping the DB cache on performance and data consistency.
Deploying sanguine-fe with
|
| Latest commit: |
6843979
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b05e7719.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-manager-cache.sanguine-fe.pages.dev |
![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.yaml
Review profile: CHILL
Files selected for processing (4)
- services/rfq/relayer/inventory/inflight.go (1 hunks)
- services/rfq/relayer/inventory/manager.go (5 hunks)
- services/rfq/relayer/inventory/options.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (1 hunks)
Additional comments not posted (15)
services/rfq/relayer/inventory/options.go (3)
5-6: Fields added tobalanceFetchOptions.The new fields
shouldRefreshBalancesandskipDBCacheare correctly added to thebalanceFetchOptionsstruct.
13-18: FunctionShouldRefreshBalancescorrectly implemented.The function sets the
shouldRefreshBalancesfield to true, aligning with the new naming convention.
20-23: FunctionSkipDBCachecorrectly implemented.The function sets the
skipDBCachefield to true, providing an option to bypass database caching.services/rfq/relayer/inventory/inflight.go (6)
13-18:inFlightManagerstruct correctly defined.The struct fields for TTL, database service, mutex, and cache entry are correctly defined.
20-24:inFlightQuoteCacheEntrystruct correctly defined.The struct fields for creation time and quotes are correctly defined.
29-34: FunctionWithTTLcorrectly implemented.The function sets the TTL for the
inFlightManager.
38-51: FunctionnewInflightManagercorrectly implemented.The function initializes the
inFlightManagerwith default TTL and applies the given options.
53-68: FunctionGetInFlightQuotescorrectly implemented.The function retrieves in-flight quotes from the database if the cache is skipped or needs refreshing, and updates the cache entry.
70-75: FunctionshouldRefreshcorrectly implemented.The function checks if the cache needs refreshing based on the TTL and cache entry creation time.
services/rfq/relayer/quoter/quoter.go (1)
242-244: Ensure the correctness ofSkipDBCacheusage.The inclusion of
inventory.SkipDBCache()ensures that the balances fetched are current and not influenced by stale data. Verify that this change does not introduce unintended side effects.Verification successful
Ensure the correctness of
SkipDBCacheusage.The inclusion of
inventory.SkipDBCache()ensures that the balances fetched are current and not influenced by stale data. TheGetCommittableBalancesfunction correctly respects theskipDBCacheoption by conditionally refreshing balances based on the provided options.
services/rfq/relayer/inventory/manager.go: Implementation ofGetCommittableBalancesrespects theskipDBCacheoption.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `SkipDBCache` in the codebase. # Test: Search for the function usage. Expect: Correct usage of `SkipDBCache`. rg --type go -A 5 $'SkipDBCache'Length of output: 964
Script:
#!/bin/bash # Description: Verify the implementation of `GetCommittableBalances` to ensure it respects `skipDBCache`. # Test: Search for the definition and usage of `GetCommittableBalances`. rg --type go -A 10 'GetCommittableBalances'Length of output: 6493
services/rfq/relayer/inventory/manager.go (5)
82-83: Addition ofinFlightQuoteManagerfield looks good.The new field
inFlightQuoteManagerhas been added to the structinventoryManagerImplto manage in-flight quotes.
111-115: Updated condition for refreshing balances looks good.The condition for refreshing balances has been updated to check
reqOptions.shouldRefreshBalancesinstead ofreqOptions.skipCache.
201-209: Initialization ofinFlightQuoteManagerlooks good.The
inFlightQuoteManageris now initialized in theNewInventoryManagerconstructor.
629-630: Introduction of local variablegasBalanceslooks good.The local variable
gasBalanceshas been introduced to handle gas token balances within therefreshBalancesmethod.
119-121: Usage ofinFlightQuoteManagerlooks good.The retrieval of in-flight quotes now uses
i.inFlightQuoteManager.GetInFlightQuotes, which leverages the new cache mechanism.However, ensure that the
GetInFlightQuotesmethod handles errors and edge cases appropriately.Verification successful
The
GetInFlightQuotesmethod handles errors and edge cases appropriately.The method implementation includes error handling for database fetch operations and leverages a cache mechanism to optimize performance.
- Location:
services/rfq/relayer/inventory/inflight.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `GetInFlightQuotes` method in `inFlightQuoteManager`. # Test: Search for the method implementation. Expect: Proper error handling and edge case management. ast-grep --lang go --pattern $'func (i *inFlightManager) GetInFlightQuotes($_, $_) { $$$ }'Length of output: 94
Script:
#!/bin/bash # Description: Search for the `GetInFlightQuotes` method in the codebase, regardless of the struct it belongs to. # Test: Search for the method implementation. Expect: Proper error handling and edge case management. rg 'func .*GetInFlightQuotes' -A 20Length of output: 1574
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces an in-flight quote cache to optimize performance by reducing the frequency of database queries.
- New Cache Mechanism: Added
services/rfq/relayer/inventory/inflight.goto manage in-flight quotes with a caching mechanism. - Manager Integration: Modified
services/rfq/relayer/inventory/manager.goto integrate the in-flight quote cache into theinventoryManagerImplstruct. - Balance Fetching Options: Updated
services/rfq/relayer/inventory/options.goto includeShouldRefreshBalancesandSkipDBCacheoptions for more granular control. - Quote Submission Optimization: Adjusted
services/rfq/relayer/quoter/quoter.goto use the new cache mechanism in theSubmitAllQuotesmethod.
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| if err != nil { | ||
| return nil, fmt.Errorf("could not get in flight quotes: %w", err) | ||
| } | ||
| q.mux.Lock() |
There was a problem hiding this comment.
style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released.
| return inFlightQuotes, nil | ||
| } | ||
|
|
||
| q.mux.RLock() |
There was a problem hiding this comment.
logic: Ensure that the read lock is held for the shortest time possible to minimize contention.
| return q.entry.quotes, nil | ||
| } | ||
|
|
||
| func (q *inFlightManager) shouldRefresh() bool { |
There was a problem hiding this comment.
style: Potential race condition if shouldRefresh is called concurrently with GetInFlightQuotes. Ensure proper locking.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/inventory/inflight.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/inventory/inflight.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces an in-flight quote cache to optimize performance by reducing the frequency of database queries.
- New Cache Mechanism: Added
services/rfq/relayer/inventory/inflight.goto manage in-flight quotes with a caching mechanism. - Manager Integration: Modified
services/rfq/relayer/inventory/manager.goto integrate the in-flight quote cache into theinventoryManagerImplstruct. - Balance Fetching Options: Updated
services/rfq/relayer/inventory/options.goto includeShouldRefreshBalancesandSkipDBCacheoptions for more granular control. - Quote Submission Optimization: Adjusted
services/rfq/relayer/quoter/quoter.goto use the new cache mechanism in theSubmitAllQuotesmethod.
2 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings
| const defaultTTL = 250 * time.Millisecond | ||
|
|
||
| // newInflightManager creates a new inFlightManager with the given options. | ||
| func newInflightManager(db reldb.Service, options ...QuoterOption) *inFlightManager { |
There was a problem hiding this comment.
logic: Consider dependency injection for the database service to facilitate testing and flexibility.
| } | ||
|
|
||
| func (q *inFlightManager) GetInFlightQuotes(ctx context.Context, skipCache bool) (quotes []reldb.QuoteRequest, err error) { | ||
| if skipCache || q.shouldRefresh() { |
There was a problem hiding this comment.
logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks.
| if err != nil { | ||
| return nil, fmt.Errorf("could not get in flight quotes: %w", err) | ||
| } | ||
| q.mux.Lock() |
There was a problem hiding this comment.
style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released.
| return inFlightQuotes, nil | ||
| } | ||
|
|
||
| q.mux.RLock() |
There was a problem hiding this comment.
logic: Ensure that the read lock is held for the shortest time possible to minimize contention.
| return q.entry.quotes, nil | ||
| } | ||
|
|
||
| func (q *inFlightManager) shouldRefresh() bool { |
There was a problem hiding this comment.
style: Potential race condition if shouldRefresh is called concurrently with GetInFlightQuotes. Ensure proper locking.
| cfg: cfg, | ||
| chainClient: clientFetcher, | ||
| txSubmitter: txSubmitter, | ||
| rebalanceManagers: rebalanceManagers, |
There was a problem hiding this comment.
style: Consider adding error handling for the new inFlightManager initialization.
| // get db first | ||
| // Add other committed, but incomplete statuses here | ||
| // TODO: clean me up: you can do this by having a IsLiquidityCommitted() method on the type. | ||
| inFlightQuotes, err := i.db.GetQuoteResultsByStatus(ctx, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted) | ||
| inFlightQuotes, err := i.inFlightQuoteManager.GetInFlightQuotes(ctx, reqOptions.skipDBCache) |
There was a problem hiding this comment.
style: Ensure inFlightQuoteManager.GetInFlightQuotes handles cache retrieval failures gracefully.
| @@ -106,14 +108,15 @@ func (i *inventoryManagerImpl) GetCommittableBalance(ctx context.Context, chainI | |||
| func (i *inventoryManagerImpl) GetCommittableBalances(ctx context.Context, options ...BalanceFetchArgOption) (res map[int]map[common.Address]*big.Int, err error) { | |||
| reqOptions := makeOptions(options) | |||
| // TODO: hard fail if cache skip breaks | |||
| if reqOptions.skipCache { | |||
| if reqOptions.shouldRefreshBalances { | |||
| // TODO; no need for this if refresh already in flight | |||
| _ = i.refreshBalances(ctx) | |||
There was a problem hiding this comment.
logic: Avoid redundant balance refreshes if a refresh is already in progress.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/relayer/inventory/inflight.go (1 hunks)
- services/rfq/relayer/inventory/manager.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/inventory/inflight.go
- services/rfq/relayer/inventory/manager.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces an in-flight quote cache to optimize performance by reducing the frequency of database queries.
- New Cache Mechanism: Added
services/rfq/relayer/inventory/inflight.goto manage in-flight quotes with a caching mechanism. - TTL Adjustment: Increased default TTL for the cache from 250 milliseconds to 2 seconds.
- Cache Logic: Updated
GetInFlightQuotesmethod to include logic for skipping and refreshing the cache. - Performance Improvement: Aims to address performance bottlenecks in
GetCommittableBalancesby reducing SQL query frequency.
1 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| } | ||
| } | ||
|
|
||
| const defaultTTL = 2 * time.Second |
There was a problem hiding this comment.
style: Increasing the default TTL to 2 seconds may reduce database load but could lead to stale data. Monitor for any issues.
| } | ||
|
|
||
| func (q *inFlightManager) GetInFlightQuotes(ctx context.Context, skipCache bool) (quotes []reldb.QuoteRequest, err error) { | ||
| if skipCache || q.shouldRefresh() { |
There was a problem hiding this comment.
logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks.
| if err != nil { | ||
| return nil, fmt.Errorf("could not get in flight quotes: %w", err) | ||
| } | ||
| q.mux.Lock() |
There was a problem hiding this comment.
style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released.
| return inFlightQuotes, nil | ||
| } | ||
|
|
||
| q.mux.RLock() |
There was a problem hiding this comment.
logic: Ensure that the read lock is held for the shortest time possible to minimize contention.
| return q.entry.quotes, nil | ||
| } | ||
|
|
||
| func (q *inFlightManager) shouldRefresh() bool { |
There was a problem hiding this comment.
style: Potential race condition if shouldRefresh is called concurrently with GetInFlightQuotes. Ensure proper locking.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/inventory/inflight.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/inventory/inflight.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces an in-flight quote cache to optimize performance by reducing the frequency of database queries.
- New Cache Mechanism: Added
services/rfq/relayer/inventory/inflight.goto manage in-flight quotes with a caching mechanism. - TTL Adjustment: Increased default TTL for the cache from 250 milliseconds to 2 seconds.
- Cache Logic: Updated
services/rfq/relayer/inventory/manager.goto include logic for skipping and refreshing the cache. - Performance Improvement: Aims to address performance bottlenecks in
GetCommittableBalancesby reducing SQL query frequency.
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| @@ -106,14 +108,15 @@ func (i *inventoryManagerImpl) GetCommittableBalance(ctx context.Context, chainI | |||
| func (i *inventoryManagerImpl) GetCommittableBalances(ctx context.Context, options ...BalanceFetchArgOption) (res map[int]map[common.Address]*big.Int, err error) { | |||
| reqOptions := makeOptions(options) | |||
| // TODO: hard fail if cache skip breaks | |||
| if reqOptions.skipCache { | |||
| if reqOptions.shouldRefreshBalances { | |||
| // TODO; no need for this if refresh already in flight | |||
| _ = i.refreshBalances(ctx) | |||
There was a problem hiding this comment.
logic: Avoid redundant balance refreshes if a refresh is already in progress.
| // get db first | ||
| // Add other committed, but incomplete statuses here | ||
| // TODO: clean me up: you can do this by having a IsLiquidityCommitted() method on the type. | ||
| inFlightQuotes, err := i.db.GetQuoteResultsByStatus(ctx, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted) | ||
| inFlightQuotes, err := i.inFlightQuoteManager.GetInFlightQuotes(ctx, reqOptions.skipDBCache) |
There was a problem hiding this comment.
style: Ensure inFlightQuoteManager.GetInFlightQuotes handles cache retrieval failures gracefully.
| cfg: cfg, | ||
| chainClient: clientFetcher, | ||
| txSubmitter: txSubmitter, | ||
| rebalanceManagers: rebalanceManagers, |
There was a problem hiding this comment.
style: Consider adding error handling for the new inFlightManager initialization.
trajan0x
force-pushed
the
fix/manager-cache
branch
from
3d62a40 to
6843979
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/inventory/manager.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/inventory/manager.go
Description
addresses #2962
Summary by CodeRabbit
New Features
inFlightManagerfor efficient and thread-safe management of in-flight quotes.Improvements