feat(bundles): retry based on retry attempts #299
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #299 +/- ##
===========================================
+ Coverage 72.40% 72.47% +0.07%
===========================================
Files 40 40
Lines 10114 10130 +16
Branches 584 585 +1
===========================================
+ Hits 7323 7342 +19
+ Misses 2787 2784 -3
Partials 4 4 ☔ View full report in Codecov by Sentry. |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error @permaweb/[email protected]: The engine "yarn" is incompatible with this module. Expected version "please-use-npm". Got "1.22.22" 📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ 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 (
|
![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
🧹 Nitpick comments (3)
src/workers/bundle-repair-worker.ts (1)
68-69: Consider keeping the retry interval configurable.The retry interval has been hardcoded to 60 seconds, removing the configurability previously provided by
config.BUNDLE_REPAIR_RETRY_INTERVAL_SECONDS. This might limit the system's adaptability to different environments or requirements.- 60 * 1000, - // config.BUNDLE_REPAIR_RETRY_INTERVAL_SECONDS * 1000, + config.BUNDLE_REPAIR_RETRY_INTERVAL_SECONDS * 1000,migrations/2025.01.30T14.12.03.bundles.add-retry-stats.sql (1)
1-7: LGTM! Consider adding documentation.The schema changes look good. The column types are appropriate, and the indexes will help optimize queries. Consider adding comments to document:
- The purpose of each column
- The expected values/format for timestamps
- The rationale for replacing
import_attempt_last_queued_idxwithimport_attempt_last_retried_idxsrc/database/standalone-sqlite.test.ts (1)
1177-1292: LGTM! Consider adding edge case tests.The test suite thoroughly covers the core functionality. Consider adding tests for:
- Maximum retry attempts (if there's a limit)
- Concurrent retry updates
- Error cases (e.g., invalid root transaction ID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
migrations/2025.01.30T14.12.03.bundles.add-retry-stats.sql(1 hunks)migrations/down/2025.01.30T14.12.03.bundles.add-retry-stats.sql(1 hunks)src/database/sql/bundles/import.sql(1 hunks)src/database/sql/bundles/repair.sql(1 hunks)src/database/standalone-sqlite.test.ts(2 hunks)src/database/standalone-sqlite.ts(3 hunks)src/types.d.ts(1 hunks)src/workers/bundle-repair-worker.ts(2 hunks)test/bundles-schema.sql(2 hunks)
🔇 Additional comments (10)
src/workers/bundle-repair-worker.ts (1)
112-112: LGTM! Proper placement of retry tracking.The
saveBundleRetriescall is correctly placed after logging but before queueing the transaction, and it's properly wrapped in the existing try-catch block for error handling.src/types.d.ts (1)
230-230: LGTM! Well-defined interface method.The
saveBundleRetriesmethod is properly typed with a clear parameter and return type, maintaining consistency with the interface's style.src/database/standalone-sqlite.ts (3)
914-920: LGTM! Proper implementation of bundle retry tracking.The worker implementation correctly:
- Decodes the rootTransactionId from base64
- Updates the bundle retry information with current timestamp
2934-2936: LGTM! Clean interface implementation.The main class implementation properly queues the write operation while maintaining consistent error handling patterns.
3356-3359: LGTM! Proper message handling integration.The worker message handling is correctly integrated into the switch statement, maintaining the established null response pattern.
migrations/down/2025.01.30T14.12.03.bundles.add-retry-stats.sql (2)
1-3: LGTM! Clean column removal.The columns are dropped in a clear and straightforward manner.
5-7: LGTM! Proper index management.The index operations properly use IF EXISTS/IF NOT EXISTS clauses to prevent errors during migration.
src/database/sql/bundles/repair.sql (1)
22-22: LGTM! The ordering change aligns with the new retry-based tracking.The change to sort by
retry_attempt_countandlast_retried_atis consistent with the schema changes and provides a good strategy for prioritizing bundle processing.src/database/sql/bundles/import.sql (1)
57-66: LGTM! The retry update logic is well-implemented.The SQL statement correctly handles:
- NULL values using COALESCE
- First-time vs subsequent retries
- Parameterized inputs for safety
test/bundles-schema.sql (1)
57-57: LGTM! Test schema matches production schema.The test schema correctly includes all the new columns and indexes from the migration.
Also applies to: 159-160
karlprieb
force-pushed
the
PE-7436-bundle-retry-counts
branch
from
d1dc6aa to
a346215
Compare
| @@ -107,6 +108,7 @@ export class BundleRepairWorker { | |||
| ); | |||
| for (const bundleId of bundleIds) { | |||
| this.log.info('Retrying failed bundle', { bundleId }); | |||
| await this.bundleIndex.saveBundleRetries(bundleId); | |||
There was a problem hiding this comment.
@djwhitt I'm awaiting here to make sure we update retry stats
|
Planning to merge this one after release 24 on Monday. |
No description provided.