suggestion: save state from temporal temporal #7253
Merged
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
|
Charles and I talked about this, and we decided to go with option 2. I will merge it into my PR and update it accordingly. |
cgardens
added
area/platform
|
@cgardens, to go further on option 2, we can just add a |
|
i think it is worth keeping them separate because we know the lifecycle of the objects is different. StandardSync is a configuration object that gets updated via user interaction. The state gets updated by the runnings syncs. |
tuliren
added a commit
that referenced
this pull request
Oct 26, 2021
* Add migration to create latest state table * Log migration name * Expose db variables to airbyte-db * Implement migration * Fix migration test * temp * Rebase on master * Save state in temporal (#7253) * Copy state to airbyte_configs table * Add standard sync state * Move state methods to config repository * Add unit tests * Fix unit tests * Register standard sync state in migration * Add comment * Use config model instead of json node * Add comments * Remove unnecessary method * Fix migration query * Remove unused config database * Move persist statement and log the call * Update dev doc * Add unit tests for sync workflow Co-authored-by: Charles <giardina.charles@gmail.com>
schlattk
pushed a commit
to schlattk/airbyte
that referenced
this pull request
Jan 4, 2022
* Add migration to create latest state table * Log migration name * Expose db variables to airbyte-db * Implement migration * Fix migration test * temp * Rebase on master * Save state in temporal (airbytehq#7253) * Copy state to airbyte_configs table * Add standard sync state * Move state methods to config repository * Add unit tests * Fix unit tests * Register standard sync state in migration * Add comment * Use config model instead of json node * Add comments * Remove unnecessary method * Fix migration query * Remove unused config database * Move persist statement and log the call * Update dev doc * Add unit tests for sync workflow Co-authored-by: Charles <giardina.charles@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Branching off this PR and thinking about a different approach: #7219
Main things I'm trying to focus on:
DefaultJobPersistence. I think it's important to keep these databases separate in the code.Option 1: Separate Abstraction for "complex" queries in the configs db.
In this PR, I added a new persistence abstraction for the config db, called
ConfigPersistence2(just ignore the bad naming for now, if we go in this direction we can fix it). The new abstraction is akin to what we do for theJobsPersistence, it takes in the database and then all the queries we write against that db can go in there. If we go with this approach we will have 3 abstractions.*
ConfigPersistence- document-database-style interface over the config database.*
ConfigRepository- syntactic sugar overConfigPersistence*
ConfigPersistence2- all non document-database-style queries.* as we normalize the config database schema, ConfigPersistence2 will get bigger and
ConfigPersistencewill get deemphasized.Option 2: treat sync state like we do other objects in the configs db
we just add the sync state "table" the
airbyte_configstable and have it look the same as all of the other configs. Then we worry about normalizing it later when we do it for the rest of the configs.If we think Option2 is the right thing for now, that's fine with me too. Option 1 leaves us in a halfsey state and if we are not going to normalize the schemas soon, then that's probably a bad thing.