feat: dal retry when init failed #260
Conversation
WalkthroughThis pull request introduces enhanced logging and initialization capabilities for MySQL data sources across multiple components. The changes primarily focus on adding optional logger support and retry mechanisms during data source initialization. A new development dependency Changes
Sequence DiagramsequenceDiagram
participant Runner
participant DalModuleLoadUnitHook
participant MysqlDataSourceManager
participant MysqlDataSource
Runner->>DalModuleLoadUnitHook: Create with optional logger
DalModuleLoadUnitHook->>MysqlDataSourceManager: Configure data source
MysqlDataSourceManager->>MysqlDataSource: Create with logger and retry options
MysqlDataSource->>MysqlDataSource: Initialize with retry mechanism
alt Initialization fails
MysqlDataSource-->>MysqlDataSource: Retry initialization
end
Possibly related PRs
Poem
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 (
|
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/dal-runtime/src/MySqlDataSource.ts (2)
11-12: Add validation checks for retry settings
Consider validating that initRetryTimes is a positive integer. This will help avoid unexpected behavior if a negative or zero value is passed in.
28-29: Use descriptive naming or doc comments for private fields
The new private fields #initRetryTimes and #logger are well-introduced. If more context is needed for future maintainers, consider adding doc comments describing their intended behavior.core/dal-runtime/test/DataSource.test.ts (1)
90-199: Consider breaking down the large test caseWhile the test is comprehensive, it's testing multiple operations (insert, update, delete, find, paginate) in a single test case. Consider breaking it down into smaller, focused test cases for better maintainability and easier debugging.
Example structure:
describe('execute', () => { // ... setup code ... describe('CRUD operations', () => { it('should successfully insert a record', async () => { // Test insert operation }); it('should successfully update a record', async () => { // Test update operation }); it('should successfully find a record', async () => { // Test find operation }); it('should successfully delete a record', async () => { // Test delete operation }); it('should handle pagination correctly', async () => { // Test pagination }); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/dal-runtime/package.json(1 hunks)core/dal-runtime/src/MySqlDataSource.ts(3 hunks)core/dal-runtime/test/DataSource.test.ts(2 hunks)plugin/dal/lib/DalModuleLoadUnitHook.ts(2 hunks)plugin/dal/lib/MysqlDataSourceManager.ts(1 hunks)standalone/standalone/src/Runner.ts(1 hunks)
🔇 Additional comments (13)
core/dal-runtime/src/MySqlDataSource.ts (3)
4-4: Ensure consistent logging interface usage
The import of Logger from '@eggjs/tegg-types' looks good. Just verify that any previously used logging mechanism or console statements are properly replaced by this new logger interface for consistency.
33-34: Constructor destructuring is straightforward
The destructuring approach is concise. Just ensure that any required fallback values or validations are handled if initRetryTimes or logger is undefined.
Also applies to: 37-37
46-49: Ensure initialization call is conditional
Line 46 unconditionally calls #doInit(1) if initSql is truthy. If there's a scenario where initSql is configured but initialization should be postponed (e.g. lazy init), consider a way to skip or delay.
core/dal-runtime/package.json (1)
60-60: Ensure test coverage for mocking with 'mm' library
The addition of the "mm" library is typically for mocking in tests. Make sure the test suite is updated to utilize it effectively, and confirm that all newly added logic (like retry) is covered by tests.
[approve]
plugin/dal/lib/DalModuleLoadUnitHook.ts (3)
2-2: Import of Logger
The Logger import is consistent with the rest of the changes. Confirm that other references to logging inside the plugin still align with this new interface.
9-9: Optional logger integration
Allowing an optional logger is a useful flexibility. Ensure that any log invocations gracefully handle a null/undefined logger to avoid runtime errors.
Also applies to: 11-11, 14-14
26-26: Passing logger in dataSourceOptions
This addition ensures that the logger is consistently injected into the DataSource. Confirm that all references to dataSourceOptions in other parts of the code handle the logger property without issues.
plugin/dal/lib/MysqlDataSourceManager.ts (1)
24-26: Configurations properly separated; consider deeper type checks
Extracting logger from config with the destructuring approach ensures code clarity. If you anticipate special usage or type constraints on logger, ensure deeper type checks or fallbacks. Otherwise, this design is flexible and clean.
Also applies to: 32-32
core/dal-runtime/test/DataSource.test.ts (4)
16-24: LGTM: Well-structured test configuration
The MySQL configuration is properly set up with essential parameters including timezone settings and initialization SQL.
31-39: LGTM: Comprehensive error handling test
Good test coverage for initialization failure scenario. The test properly verifies both the error propagation and the execution of initialization SQL.
41-51: LGTM: Thorough retry mechanism test
The test effectively verifies that initialization retries the specified number of times before giving up.
53-66: LGTM: Well-implemented recovery test
Good test coverage for the successful recovery scenario after an initial failure.
standalone/standalone/src/Runner.ts (1)
172-176: LGTM: Proper logger initialization and injection
Good implementation of logger extraction with fallback to console. The logger is correctly passed to all DAL-related hooks for consistent logging behavior.
|
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
MysqlDataSource,DalModuleLoadUnitHook, andRunner.MysqlDataSource.Bug Fixes
MysqlDataSource.Tests