This repository has been archived by the owner on Sep 23, 2024. It is now read-only.
Draft: Fix FULL_TABLE replication to comply with Singer spec #96
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Singer Spec clearly states that FULL_TABLE replication mode must replicate "all available records dating back to a `start_date`". This tap erroneously switches to an incremental replication strategy if it detects an auto-incrementing primary key. This is both unsafe (it will omit updates to existing rows) and in violation of the spec. Remove this auto-incremental-update logic from the full-table mode.
paultiplady
changed the title
paultiplady
changed the title
paultiplady
commented
Mar 1, 2022
| @@ -18,7 +18,7 @@ def generate_bookmark_keys(catalog_entry): | |||
| stream_metadata = md_map.get((), {}) | |||
| replication_method = stream_metadata.get('replication-method') | |||
|
|
|||
| base_bookmark_keys = {'last_pk_fetched', 'max_pk_values', 'version', 'initial_full_table_complete'} | |||
| base_bookmark_keys = {'last_pk_fetched', 'version', 'initial_full_table_complete'} | |||
There was a problem hiding this comment.
TODO: Remove last_pk_fetched too.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the bug
When it detects an incrementing PK, the "full table" sync mode actually runs an incremental sync with an ephemeral "last_pk_fetched" that is computed at the beginning of the extraction, stashed in the singer state, and then cleared at the end of the extraction. This seems to be an optimization to allow the tap to be retried and resume on failures. However it's not conformant to the Singer spec, and seems to break Meltano.
Due to an issue in Meltano (it seems that clear_bookmark isn't correctly clearing the key from future jobs' state), the tap is being started with stale last_pk_fetched bookmarks.
This means changes will be missed where rows have been updated, as the tap will just skip to the rows with new PKs. I believe this breaks compatibility with the Singer spec on how sync-mode is supposed to work: https://github.com/singer-io/getting-started/blob/master/docs/SYNC_MODE.md#replication-method.
Expected behavior
Full table mode should perform a full table extraction, as described in the spec and this tap's docs: "Full-table replication extracts all data from the source table each time the tap is invoked."
The Singer spec says full table mode should depend ONLY on the start_date in the config, and replicates all available records for every sync.
FIXES: #91