add a default TTL to non-periodic queries, and GC stale queries #1812
Conversation
joe-bowman
requested review from
faddat,
ajansari95 and
arhamchordia
as code owners
|
""" WalkthroughThe changes introduce a new constant Changes
Poem
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/interchainquery/keeper/abci.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
x/interchainquery/keeper/abci.go
7-7: File is not gci-ed with --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order
(gci)
27-27: File is not gofumpt-ed
(gofumpt)
🪛 GitHub Check: devskim
x/interchainquery/keeper/abci.go
[notice] 27-27: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
🪛 GitHub Actions: golangci-lint
x/interchainquery/keeper/abci.go
[error] 7-7: File is not properly formatted with gci. Required format: --skip-generated -s standard -s default -s blank -s dot -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) -s prefix(github.com/cometbft/cometbft) -s prefix(github.com/cosmos/ibc-go) -s prefix(github.com/quicksilver-zone/quicksilver) --custom-order
[error] 27-27: File is not properly formatted according to gofumpt standards
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
🔇 Additional comments (1)
x/interchainquery/keeper/abci.go (1)
14-14: LGTM: DefaultTTL constant.The value of 1000 blocks for the TTL seems reasonable for managing stale queries.
| if !queryInfo.Period.IsNegative() && // not periodic | ||
| !queryInfo.LastEmission.IsNil() && // has been emitted | ||
| !queryInfo.LastEmission.IsZero() && // has been emitted | ||
| queryInfo.LastEmission.LT(math.NewInt(ctx.BlockHeight()-DefaultTTL)) { // TODO: add per query TTL |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/interchainquery/keeper/abci.go (1)
28-28: Consider implementing per-query TTL.The TODO comment suggests adding per-query TTL. This would provide more flexibility in query management, allowing different TTLs for different types of queries.
Would you like me to help implement this enhancement? This could involve:
- Adding a TTL field to the Query struct
- Using the query-specific TTL if set, otherwise falling back to DefaultTTL
🧰 Tools
🪛 GitHub Check: devskim
[notice] 28-28: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(4 hunks)x/interchainquery/keeper/abci.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: devskim
x/interchainquery/keeper/abci.go
[notice] 28-28: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
🔇 Additional comments (3)
x/interchainquery/keeper/abci.go (2)
15-15: LGTM!The constant value of 1000 blocks for the default TTL is reasonable and the naming follows Go conventions.
24-31: 🛠️ Refactor suggestionImprove query deletion logic for better clarity and maintainability.
The current implementation has a few areas for improvement:
- Using
Period.IsNegative()to check for non-periodic queries is counterintuitive- The double check for emission (
!IsNil && !IsZero) seems redundantConsider this refactoring:
- if !queryInfo.Period.IsNegative() && // not periodic - !queryInfo.LastEmission.IsNil() && // has been emitted - !queryInfo.LastEmission.IsZero() && // has been emitted - queryInfo.LastEmission.LT(math.NewInt(ctx.BlockHeight()-DefaultTTL)) { //TODO: add per query TTL + // Delete non-periodic queries that haven't been re-emitted within TTL + if queryInfo.Period.IsZero() && // non-periodic query + !queryInfo.LastEmission.IsNil() && // has been emitted at least once + queryInfo.LastEmission.LT(math.NewInt(ctx.BlockHeight()-DefaultTTL)) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: devskim
[notice] 28-28: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious commentgo.mod (1)
69-69: LGTM!The dependency version changes appear to be routine version management and don't directly impact the TTL implementation.
Also applies to: 152-152, 179-179, 265-265
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1812 +/- ##
==========================================
- Coverage 63.43% 63.40% -0.04%
==========================================
Files 172 172
Lines 15449 15457 +8
==========================================
Hits 9800 9800
- Misses 4838 4846 +8
Partials 811 811
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Periodically, ICQ queries can be emitted that - because of external factors (rpc node pruning, etc.) - cannot be satisfied. This leads to stale queries that do not get cleaned up and cause a drop in performance, as the ICQ relayer repeatedly tries to deliver them.
This PR adds a Default TTL (consider adding a per query TTL in a future release), so that stale queries can be garbage collected.
Summary by CodeRabbit
New Features
Chores