[source-mssql/mysql/postgres] Fix and cleanup oc map #42024
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
xiaohansong
commented
Jul 16, 2024
...rbyte-cdk/dependencies/src/main/kotlin/io/airbyte/commons/exceptions/ConfigErrorException.kt
Outdated
Show resolved
Hide resolved
|
@xiaohansong , also fixes this https://github.com/airbytehq/airbyte-internal-issues/issues/8560 ? |
Yes - thanks for linking that! I'll update details in that ticket. |
rodireich
reviewed
Jul 19, 2024
| @@ -159,7 +159,7 @@ public static boolean isSavedOffsetStillPresentOnServer(final JdbcDatabase datab | |||
| AirbyteTraceMessageUtility.emitAnalyticsTrace(cdcCursorInvalidMessage()); | |||
| if (!sourceConfig.get("replication_method").has(INVALID_CDC_CURSOR_POSITION_PROPERTY) || sourceConfig.get("replication_method").get( | |||
| INVALID_CDC_CURSOR_POSITION_PROPERTY).asText().equals(FAIL_SYNC_OPTION)) { | |||
| throw new ConfigErrorException( | |||
| throw new ConfigErrorException("test", | |||
There was a problem hiding this comment.
ah yeah just to bypass the compile error when I draft this PR. I'll revert this!
| @@ -41,7 +41,13 @@ public void updatePrimaryKeyLoadState(final AirbyteStreamNameNamespacePair pair, | |||
|
|
|||
| // Returns the previous state emitted, represented as a {@link PrimaryKeyLoadStatus} associated with | |||
| // the stream. | |||
| public abstract PrimaryKeyLoadStatus getPrimaryKeyLoadStatus(final AirbyteStreamNameNamespacePair pair); | |||
| public PrimaryKeyLoadStatus getPrimaryKeyLoadStatus(final AirbyteStreamNameNamespacePair pair) { | |||
| if (pairToPrimaryKeyLoadStatus.containsKey(pair)) { | |||
There was a problem hiding this comment.
simply doing get() does the same thing, no?
There was a problem hiding this comment.
yeah you are right... I was paranoid that this was null
…der` (#38829) Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com> Co-authored-by: Natik Gadzhi <natik@respawn.io>
rodireich
approved these changes
Jul 22, 2024
|
/publish-java-cdm |
|
/publish-java-cdk
|
xiaohansong
added a commit
that referenced
this pull request
Jul 23, 2024
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com> Co-authored-by: btkcodedev <btk.codedev@gmail.com> Co-authored-by: Artem Inzhyyants <36314070+artem1205@users.noreply.github.com> Co-authored-by: Natik Gadzhi <natik@respawn.io> Co-authored-by: artem1205 <artem1205@users.noreply.github.com> Co-authored-by: Augustin <augustin@airbyte.io> Co-authored-by: Antonio Papa <antoniogpapa@gmail.com> Co-authored-by: Adam Marcus <marcua@marcua.net> Co-authored-by: Bryce Groff <bgroff@hawaii.edu> Co-authored-by: Akash Kulkarni <113392464+akashkulk@users.noreply.github.com> Co-authored-by: Patrick Nilan <nilan.patrick@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.
What
https://github.com/airbytehq/oncall/issues/5825
We need to pre-populate all ocInfo or pkInfo to state manager.
In case like this:
We have 2 resumable full refreshes. It failed when we are syncing the 1st one. The final state would be like
{stream: A, streamState: { ...(detailed state)... } }
{stream: B, streamState: null }
The next time it load, with previous logic, it will only load for pkInfo(ocInfo) for stream A. When we run load records for stream B, pkInfo or ocInfo would be null. That is confirmed by the added test.
How
Added a test to cover this. There are some cdk traffic about error message and wass interface change - I just stubbed them. will wait them to merge before checking this in.
Review guide
User Impact
Can this PR be safely reverted and rolled back?