-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Avoid write empty state #2844
fix: Avoid write empty state #2844
Conversation
Reviewer's Guide by SourceryThis pull request prevents the writing of empty state messages, which could lead to data loss. The changes ensure that state messages are only written if the state is not empty, or if the current state is different from the last emitted state. Sequence diagram for state message handling in MeltanosequenceDiagram
participant Extractor
participant StateHandler
participant Database
Note over Extractor,Database: Before Fix
Extractor->>StateHandler: Start extraction
StateHandler->>Database: Write empty state {}
Note right of Database: State is lost if extraction fails
Note over Extractor,Database: After Fix
Extractor->>StateHandler: Start extraction
alt State is not empty
StateHandler->>Database: Write state message
else State is empty
Note over StateHandler: Skip writing empty state
end
State diagram for state message handling logicstateDiagram-v2
[*] --> CheckState
CheckState --> EvaluateConditions: State exists
CheckState --> [*]: Empty state
EvaluateConditions --> WriteState: State not flushed AND
State different from last
EvaluateConditions --> [*]: Conditions not met
WriteState --> UpdateLastEmitted: Write state message
UpdateLastEmitted --> [*]: Copy state
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2844 +/- ##
=======================================
Coverage 91.33% 91.33%
=======================================
Files 62 62
Lines 5205 5206 +1
Branches 671 672 +1
=======================================
+ Hits 4754 4755 +1
Misses 319 319
Partials 132 132 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #2844 will not alter performanceComparing Summary
|
Hey @joaopamaral, any particular reason for closing this? |
Hi @edgarrmondragon, I'll finish adding the test cases before reopen it! Maybe I should just open as draft 😅 |
@@ -138,6 +140,7 @@ def test_sync_sqlite_to_sqlite( | |||
new_lines = new_stdout.readlines() | |||
assert len(orig_lines) > 0, "Orig tap output should not be empty." | |||
assert len(new_lines) > 0, "(Re-)tapped target output should not be empty." | |||
assert orig_lines[0] == new_lines[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was incorrect before because the state was not included in the provided state to the second call.
- {"type":"STATE","value":{}}
+ {"type":"STATE","value":{"bookmarks":{"main-t0":{"replication_key":"c1","replication_key_value":55}}}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @joaopamaral - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Edgar Ramírez Mondragón <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @joaopamaral!
Summary
Currently, Meltano sends an empty state message when no state is provided in the
--state
parameter. The issue arises when the state message is processed and the database is updated. If the Meltano extraction fails before the next state message is sent, the state in the database will be empty, leading to the loss of the last known state.This issue has been observed across several extractors using PostgreSQL on Cloud SQL (GCP) as the Meltano database, forcing manual updates to restore the state.
Bug Fixes:
📚 Documentation preview 📚: https://meltano-sdk--2844.org.readthedocs.build/en/2844/
Summary by Sourcery
Bug Fixes: