DMS: Add option ignore_ddl to ignore DDL events
#119
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 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. 📒 Files selected for processing (4)
WalkthroughA new boolean flag, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DMSTranslatorCrateDB
participant DMSTranslatorCrateDBRecord
participant SkipOperation
User->>DMSTranslatorCrateDB: Initialize (ignore_ddl=True)
DMSTranslatorCrateDB->>DMSTranslatorCrateDBRecord: Process DMS event
DMSTranslatorCrateDBRecord->>DMSTranslatorCrateDBRecord: to_sql()
alt Event is DDL (create-table or drop-table) and ignore_ddl=True
DMSTranslatorCrateDBRecord->>SkipOperation: Raise SkipOperation("DDL event ignored")
else Event is not DDL or ignore_ddl=False
DMSTranslatorCrateDBRecord->>DMSTranslatorCrateDBRecord: Generate SQL as normal
end
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commons_codec/transform/aws_dms.py (1)
258-258: Consider the implications of defaultingignore_ddl=True.The default value means DDL events will be ignored by default, which might be unexpected for users who assume DDL events would be processed. Consider whether this default aligns with typical user expectations or if it should default to
False.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)src/commons_codec/model.py(1 hunks)src/commons_codec/transform/aws_dms.py(4 hunks)
🔇 Additional comments (5)
src/commons_codec/model.py (1)
93-95: LGTM! Clean exception class implementation.The
SkipOperationexception is appropriately simple and well-placed in the module structure.src/commons_codec/transform/aws_dms.py (3)
14-14: Import added correctly.The
SkipOperationimport is properly added to the imports section.
95-96: Verify exception handling for SkipOperation.The
SkipOperationexceptions are raised appropriately for DDL events whenignore_ddlis enabled. Ensure that calling code properly handles these exceptions to prevent application crashes.#!/bin/bash # Description: Verify SkipOperation exception is handled in calling code # Expected: Find try-catch blocks or exception handling for SkipOperation # Search for SkipOperation exception handling rg -A 5 -B 5 "SkipOperation|except.*Skip" --type py # Look for usage of DMSTranslatorCrateDB.to_sql method ast-grep --pattern $'$_.to_sql($_)'Also applies to: 105-106
87-91: Verify DML operations can function without primary key information.The conditional primary key extraction makes sense when ignoring DDL, but ensure that subsequent DML operations (insert, update, delete) can handle cases where primary key information isn't available due to ignored DDL events.
#!/bin/bash # Description: Check if DML operations handle missing primary keys gracefully # Expected: Find error handling or validation for missing primary keys in DML operations # Search for primary key validation in DML operations rg -A 10 -B 5 "not.*primary_keys|primary_keys.*empty|primary_keys.*None" --type py # Look for usage of primary_keys in update/delete operations ast-grep --pattern $'if self.operation == "update": $$$ ' ast-grep --pattern $'if self.operation == "delete": $$$ 'CHANGES.md (1)
4-4: Clear and accurate changelog entry.The changelog appropriately documents the new
ignore_ddloption for the DMS component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commons_codec/transform/aws_dms.py (1)
95-96: Add test coverage for the new DDL skip functionality.The static analysis correctly identifies that the new skip logic (lines 96 and 106) lacks test coverage. This is important functionality that should be tested to ensure it works correctly.
Consider adding unit tests that verify:
- When
ignore_ddl=True, DDL operations raiseSkipOperation- When
ignore_ddl=False(default), DDL operations work normally- The exception messages are correct for each operation type
Would you like me to help generate test cases for this functionality?
Also applies to: 105-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)src/commons_codec/model.py(1 hunks)src/commons_codec/transform/aws_dms.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commons_codec/model.py
- CHANGES.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/commons_codec/transform/aws_dms.py (1)
src/commons_codec/model.py (3)
SkipOperation(93-94)PrimaryKeyStore(42-43)ColumnTypeMapStore(46-78)
🪛 GitHub Check: codecov/patch
src/commons_codec/transform/aws_dms.py
[warning] 96-96: src/commons_codec/transform/aws_dms.py#L96
Added line #L96 was not covered by tests
[warning] 106-106: src/commons_codec/transform/aws_dms.py#L106
Added line #L106 was not covered by tests
🔇 Additional comments (4)
src/commons_codec/transform/aws_dms.py (4)
14-14: LGTM: Import addition is correct.The
SkipOperationimport is properly placed and aligns with the exception usage in the DDL handling logic.
258-258: LGTM: Constructor parameter addition is well-implemented.The
ignore_ddlparameter with a default value ofFalsepreserves backward compatibility while providing the new functionality. The type hint and naming are appropriate.Also applies to: 262-262
87-91: LGTM: Conditional primary key processing logic is appropriate.The logic correctly prevents processing DDL-related primary key information when
ignore_ddlis enabled, maintaining consistency with the overall feature behavior.
95-96: LGTM: DDL skip logic is consistently implemented.The skip logic for both
create-tableanddrop-tableoperations follows a consistent pattern with clear, descriptive exception messages.Also applies to: 105-106
About
Provide an option to run the DMS event processor without processing DMS control events of the DDL kind.
References