Skip to content
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

Source Intercom: Migrate to manifest only format with components #47240

Merged
merged 26 commits into from
Dec 17, 2024

Conversation

btkcodedev
Copy link
Collaborator

@btkcodedev btkcodedev commented Oct 22, 2024

What

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/11089
Source Intercom: Migrate to manifest-only formats with components

How

Migrated to inline schemas
Migrated spec
Migrated to manifest only with ci

Migration report

image

@btkcodedev btkcodedev self-assigned this Oct 22, 2024
Copy link

vercel bot commented Oct 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 17, 2024 10:28pm

@btkcodedev
Copy link
Collaborator Author

btkcodedev commented Oct 22, 2024

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (6800bbf)

Copy link
Contributor

@ChristoGrab ChristoGrab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@btkcodedev Danylo and I were going over the changes and it does seem like streams which were previously reading successfully are now throwing 422 errors (company_attributes stream is an example). I recommend running the connector locally to test it out, it's possible the issue is with the custom component code not functioning after the CDK upgrade, we may need to make some updates. Let us know if you have any trouble with testing!

@octavia-squidington-iv octavia-squidington-iv requested a review from a team December 12, 2024 20:26
@btkcodedev
Copy link
Collaborator Author

@ChristoGrab I've made some changes and tested the streams in local, removed some unwanted parameters and fully revised code to new version
Errors cleared!

@ChristoGrab
Copy link
Contributor

ChristoGrab commented Dec 13, 2024

@btkcodedev Looks like we're getting very close! CAT tests show both the incremental substreams (conversation_parts and company_segments) failing on the future-state test. Looking at the abnormal_state file, I think the failure is at least in part due to the way the state object is constructed in the sample. Since they're substreams, I would expect the state object to use per-partition states, and look something like:

  {
    "type": "STREAM",
    "stream": {
      "stream_descriptor": {
        "name": "company_segments"
      },
      "stream_state": {
        "states": [
          {
            "partition": {
              "id": "<some_parent_id>",
              "parent_slice": {}
            },
            "cursor": {
              "updated_at": 9626086649
            }
          },
          {
            "partition": {
              "id": "<some_other_parent_id>",
              "parent_slice": {}
            },
            "cursor": {
              "updated_at": 9626086649
            }
          }
        ]
      }
    }
  }    

This is probably a good starting point to investigate to make sure the acceptance test is giving us accurate results.

As a heads up, I tried playing with the state for company_segments, but was still seeing the same records returned, so I don't think this is just a problem with the sample state in the tests.

@btkcodedev
Copy link
Collaborator Author

@ChristoGrab Necessary tests passed with Intercom,
Please continue review
I think its ready.

@btkcodedev
Copy link
Collaborator Author

Aha, We've got an interesting condition here @ChristoGrab.
If we change the base image to 6.10.0, Then unit tests are passing, but the incremental sync is failing while checking in abnormal values
Conversely, if the base image is 5.14.0, then acceptance tests and abnormal large value checks are passing, but the unit tests fail. 😸

So my opinion is to remove the unit tests as it is purely manifest with components right now.

@ChristoGrab
Copy link
Contributor

ChristoGrab commented Dec 17, 2024

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (c7dc09f)

@ChristoGrab
Copy link
Contributor

@btkcodedev Taking a look! You're right, and I think the issue may be that we switched to using concurrent reads in CDK 6 by default, which may have an impact on incremental substream behavior. I commented out the unit tests for now, running a fresh round of tests to verify.

Copy link
Contributor

@ChristoGrab ChristoGrab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as a release candidate for progressive rollout. Thanks for your hard work @btkcodedev!

@ChristoGrab ChristoGrab merged commit 39abff0 into master Dec 17, 2024
33 of 34 checks passed
@ChristoGrab ChristoGrab deleted the btkcodedev/intercomMigrateManifestOnly branch December 17, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/intercom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants