🐛 source-mssql: support `Read Committed` snapshot isolation level #28545

sh4sh · 2023-07-20T21:21:59Z

What

How

This PR modifies two functions getSnapshotIsolationConfig, getDataToSyncConfig and their applicable tests so that they are able to access the current connector config values.

🚨 User Impact 🚨

This is a patch fix and should not cause breaking changes.

Pre-merge Actions

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

github-actions · 2023-07-20T21:22:20Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

sh4sh · 2023-07-20T21:22:36Z

...nnectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlCdcHelper.java

@@ -120,8 +120,8 @@ static boolean isCdc(final JsonNode config) {
  @VisibleForTesting
  static SnapshotIsolation getSnapshotIsolationConfig(final JsonNode config) {
    // new replication method config since version 0.4.0
-    if (config.hasNonNull(REPLICATION_FIELD)) {
-      final JsonNode replicationConfig = config.get(REPLICATION_FIELD);
+    if (config.hasNonNull(LEGACY_REPLICATION_FIELD) && config.get(LEGACY_REPLICATION_FIELD).isObject()) {


Maybe I should also add isText case to these as well?

.isTextual() is for when replication_method was a string
.isObject() I think is suitable for a oneOf selection field.
Current implementation is with oneOf so it sounds like this is correct (?)

sh4sh · 2023-07-20T21:30:43Z

todo: bump version (patch) and add changelog

octavia-squidington-iii · 2023-07-20T21:39:46Z

source-mssql-strict-encrypt test report (commit `9c7d5a46cb`) - ✅

⏲️ Total pipeline duration: 16mn05s

Step	Result

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql-strict-encrypt test

octavia-squidington-iii · 2023-07-20T21:59:20Z

source-mssql test report (commit `9c7d5a46cb`) - ✅

⏲️ Total pipeline duration: 19mn22s

Step	Result

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql test

rodireich · 2023-07-21T22:22:41Z

@sh4sh This Looks good to me.
I just wonder if the pre-0.4.0 support is still something we need to keep.
Together with the back and forth on "replication_method" to "replication" and back, This is the most confusing piece of code

octavia-squidington-iii · 2023-07-24T15:37:37Z

source-mssql test report (commit `60ccd4880d`) - ✅

⏲️ Total pipeline duration: 46.65s

Step	Result

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql test

marcosmarxm · 2023-07-24T15:39:45Z

I said to Sunny probably we can work in a next PR to remove all confusing code between legacy and the other one.

octavia-squidington-iii · 2023-07-24T15:53:57Z

source-mssql-strict-encrypt test report (commit `60ccd4880d`) - ✅

⏲️ Total pipeline duration: 16mn11s

Step	Result

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql-strict-encrypt test

octavia-squidington-iii · 2023-07-24T16:13:19Z

source-mssql test report (commit `1b88d0ec43`) - ✅

⏲️ Total pipeline duration: 18mn01s

Step	Result

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql test

octavia-squidington-iii · 2023-07-24T16:28:57Z

source-mssql-strict-encrypt test report (commit `1b88d0ec43`) - ❌

⏲️ Total pipeline duration: 15mn28s

Step	Result
Validate airbyte-integrations/connectors/source-mssql-strict-encrypt/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Build connector tar	✅
Build source-mssql-strict-encrypt docker image for platform linux/x86_64	✅
./gradlew :airbyte-integrations:connectors:source-mssql-strict-encrypt:integrationTest	❌
Acceptance tests	❌

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql-strict-encrypt test

sh4sh · 2023-07-26T21:13:11Z

Until we figure out what's up with the tests, I published a pre-release here for the user to try out: https://hub.docker.com/layers/airbyte/source-mssql/1.1.1-dev.91cb51d107/images/sha256-ae1f56d0b8a3a1a059868156c63f256572a4be55cc0ad7968ae3c6aac579c889

sh4sh · 2023-07-27T15:05:32Z

The user who reported this issue tested the fix and confirmed it works 👍

octavia-squidington-iii · 2023-08-01T20:03:12Z

source-mssql-strict-encrypt test report (commit `4e9738166e`) - ❌

⏲️ Total pipeline duration: 12mn14s

Step	Result
Validate airbyte-integrations/connectors/source-mssql-strict-encrypt/metadata.yaml	✅
Connector version semver check	✅
QA checks	❌
Build connector tar	✅
Build source-mssql-strict-encrypt docker image for platform linux/x86_64	✅
./gradlew :airbyte-integrations:connectors:source-mssql-strict-encrypt:integrationTest	❌
Acceptance tests	❌

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql-strict-encrypt test

sh4sh · 2023-08-01T21:37:12Z

/approve-and-merge reason="bug fix, tested separately as connector currently failing on master"

octavia-approvington · 2023-08-01T21:37:43Z

It's time

octavia-squidington-iii · 2023-08-02T01:08:31Z

source-mssql test report (commit `4e9738166e`) - ❌

⏲️ Total pipeline duration: 305mn05s

Step	Result
Validate airbyte-integrations/connectors/source-mssql/metadata.yaml	✅
Connector version semver check	✅
QA checks	❌
Build connector tar	✅
Build source-mssql docker image for platform linux/x86_64	✅
./gradlew :airbyte-integrations:connectors:source-mssql:integrationTest	❌
Acceptance tests	❌

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-mssql test

…8545) * make getSnapshotIsolationConfig grab the correct config * add object case for getDataToSyncConfig and getSnapshotIsolationConfig * update tests to use current config parameters * bump connector version * add changelog

sh4sh added 3 commits July 20, 2023 15:34

make getSnapshotIsolationConfig grab the correct config

dfdb9e5

add object case for getDataToSyncConfig and getSnapshotIsolationConfig

800c4d9

update tests to use current config parameters

9c7d5a4

sh4sh requested a review from a team as a code owner July 20, 2023 21:21

octavia-squidington-iii added area/connectors Connector related issues connectors/source/mssql labels Jul 20, 2023

sh4sh commented Jul 20, 2023

View reviewed changes

sh4sh requested a review from marcosmarxm July 20, 2023 21:29

sh4sh mentioned this pull request Jul 20, 2023

🐛 source-mssql: connector forces snapshot isolation level #28543

Closed

1 task

sh4sh changed the title ~~🐛 source-mssql: support read committed snapshot isolation level~~ 🐛 source-mssql: support Read Committed snapshot isolation level Jul 20, 2023

rodireich approved these changes Jul 21, 2023

View reviewed changes

bump connector version

60ccd48

add changelog

1b88d0e

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jul 24, 2023

sh4sh added the checklist-action-run label Jul 24, 2023

Merge branch 'master' into sh4sh/mssql-snapshot-isolation

91cb51d

Merge branch 'master' into sh4sh/mssql-snapshot-isolation

4e97381

octavia-approvington approved these changes Aug 1, 2023

View reviewed changes

octavia-approvington merged commit 6df99b2 into master Aug 1, 2023

octavia-approvington deleted the sh4sh/mssql-snapshot-isolation branch August 1, 2023 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 source-mssql: support `Read Committed` snapshot isolation level #28545

🐛 source-mssql: support `Read Committed` snapshot isolation level #28545

sh4sh commented Jul 20, 2023

github-actions bot commented Jul 20, 2023 •

edited by sh4sh

Loading

sh4sh Jul 20, 2023 •

edited

Loading

rodireich Jul 21, 2023

sh4sh commented Jul 20, 2023 •

edited

Loading

octavia-squidington-iii commented Jul 20, 2023

octavia-squidington-iii commented Jul 20, 2023

rodireich commented Jul 21, 2023

octavia-squidington-iii commented Jul 24, 2023

marcosmarxm commented Jul 24, 2023

octavia-squidington-iii commented Jul 24, 2023

octavia-squidington-iii commented Jul 24, 2023

octavia-squidington-iii commented Jul 24, 2023

sh4sh commented Jul 26, 2023

sh4sh commented Jul 27, 2023

octavia-squidington-iii commented Aug 1, 2023

sh4sh commented Aug 1, 2023

octavia-approvington commented Aug 1, 2023

octavia-squidington-iii commented Aug 2, 2023

🐛 source-mssql: support Read Committed snapshot isolation level #28545

🐛 source-mssql: support Read Committed snapshot isolation level #28545

Conversation

sh4sh commented Jul 20, 2023

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

github-actions bot commented Jul 20, 2023 • edited by sh4sh Loading

Before Merging a Connector Pull Request

sh4sh Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

rodireich Jul 21, 2023

Choose a reason for hiding this comment

sh4sh commented Jul 20, 2023 • edited Loading

octavia-squidington-iii commented Jul 20, 2023

source-mssql-strict-encrypt test report (commit 9c7d5a46cb) - ✅

octavia-squidington-iii commented Jul 20, 2023

source-mssql test report (commit 9c7d5a46cb) - ✅

rodireich commented Jul 21, 2023

octavia-squidington-iii commented Jul 24, 2023

source-mssql test report (commit 60ccd4880d) - ✅

marcosmarxm commented Jul 24, 2023

octavia-squidington-iii commented Jul 24, 2023

source-mssql-strict-encrypt test report (commit 60ccd4880d) - ✅

octavia-squidington-iii commented Jul 24, 2023

source-mssql test report (commit 1b88d0ec43) - ✅

octavia-squidington-iii commented Jul 24, 2023

source-mssql-strict-encrypt test report (commit 1b88d0ec43) - ❌

sh4sh commented Jul 26, 2023

sh4sh commented Jul 27, 2023

octavia-squidington-iii commented Aug 1, 2023

source-mssql-strict-encrypt test report (commit 4e9738166e) - ❌

sh4sh commented Aug 1, 2023

octavia-approvington commented Aug 1, 2023

octavia-squidington-iii commented Aug 2, 2023

source-mssql test report (commit 4e9738166e) - ❌

🐛 source-mssql: support `Read Committed` snapshot isolation level #28545

🐛 source-mssql: support `Read Committed` snapshot isolation level #28545

github-actions bot commented Jul 20, 2023 •

edited by sh4sh

Loading

sh4sh Jul 20, 2023 •

edited

Loading

sh4sh commented Jul 20, 2023 •

edited

Loading

source-mssql-strict-encrypt test report (commit `9c7d5a46cb`) - ✅

source-mssql test report (commit `9c7d5a46cb`) - ✅

source-mssql test report (commit `60ccd4880d`) - ✅

source-mssql-strict-encrypt test report (commit `60ccd4880d`) - ✅

source-mssql test report (commit `1b88d0ec43`) - ✅

source-mssql-strict-encrypt test report (commit `1b88d0ec43`) - ❌

source-mssql-strict-encrypt test report (commit `4e9738166e`) - ❌

source-mssql test report (commit `4e9738166e`) - ❌