Change the ticket aggregation batch size to 500 #6843
Conversation
This is to workaround a CBOR coded limitation for now.
📝 WalkthroughWalkthroughThis pull request modifies the constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
db/sql/src/tickets.rs (3)
863-865: Consider adding a comment explaining the batch size limit.Adding a comment explaining why the batch size is limited to 500 would help future maintainers understand the CBOR coding limitation constraint.
.filter(ticket::Column::State.ne(AcknowledgedTicketStatus::BeingAggregated as u8)) .order_by_asc(ticket::Column::Index)// tickets must be sorted by indices in ascending order +// Limited to 500 tickets per batch due to CBOR coding limitations .limit(MAX_TICKETS_TO_AGGREGATE_BATCH)
669-671: Consider implementing the TODO for persisting mechanism hint.The TODO comment suggests implementing a hint for the persisting mechanism. This could help optimize the persistence of ticket indices.
Would you like me to help implement the persisting mechanism hint or create an issue to track this improvement?
1067-1070: Remove unused variable warning suppression.The
#[allow(unused_variables)]attribute can be removed since the variable is actually used in the code.-#[allow(unused_variables)] let current_ticket_index_from_aggregated_ticket = U256::from(aggregated_ticket.verified_ticket().index).add(aggregated_ticket.verified_ticket().index_offset);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
db/sql/src/tickets.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-smart-contracts
- GitHub Check: Linter
🔇 Additional comments (4)
db/sql/src/tickets.rs (4)
47-48: LGTM: Reducing batch size to work around CBOR coding limitation.The reduction of
MAX_TICKETS_TO_AGGREGATE_BATCHfrom 10,000 to 500 is a reasonable workaround for the CBOR coding limitation. The smaller batch size should help prevent any encoding/decoding issues while still maintaining efficient batch processing.
681-683: Consider implementing the TODO for persisting mechanism hint.Similar TODO comment for persisting mechanism hint in another function.
693-694: Consider implementing the TODO for persisting mechanism hint.Similar TODO comment for persisting mechanism hint in another function.
1-3363: Verify test coverage for the new batch size limit.The test suite is comprehensive but should verify that the new 500 ticket batch size limit is properly enforced and doesn't cause any issues.
This is to workaround a CBOR codec limitation for now.