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: Update with latest CDK features, remove custom incremental sync components #53187

Merged
merged 23 commits into from
Feb 26, 2025

Conversation

btkcodedev
Copy link
Collaborator

@btkcodedev btkcodedev commented Feb 5, 2025

What

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/11532

How

  • source-intercom is running on the latest version of CDK v6 (6.33.0+)
  • The companies stream does not use the IncrementalSingleSliceCursor
  • The contacts stream does not use the IncrementalSingleSliceCursor
  • The conversations stream does not use the IncrementalSingleSliceCursor
  • The segments stream does not use the IncrementalSingleSliceCursor
  • The company_segments stream does not use the IncrementalSubstreamSlicerCursor
  • The conversation_parts stream does not use the IncrementalSubstreamSlicerCursor
  • IncrementalSingleSliceCursor and IncrementalSubstreamSlicerCursor is deleted from components.py
  • All tests in /unit_tests pass.
  • All CI checks are passing ✅
  • Live traffic regression test suite run with passing results
  • Progressive rollout starting with 20%
  • Progressive rollout starting with 50%
  • Progressive rollout as main release of the candidate version

Copy link

vercel bot commented Feb 5, 2025

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 Feb 26, 2025 2:21pm

@btkcodedev
Copy link
Collaborator Author

btkcodedev commented Feb 5, 2025

/bump-version type="minor" changelog="Update with latest CDK features, remove custom incremental sync components"

Bump Version job started... Check job output.

✅ Changes applied successfully. (1b27557)

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Feb 5, 2025
@btkcodedev
Copy link
Collaborator Author

btkcodedev commented Feb 5, 2025

/format-fix

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

✅ Changes applied successfully. (2584cfa)

@btkcodedev
Copy link
Collaborator Author

Removed parameters field, and other redundant fields for more compatibility with builder

@btkcodedev
Copy link
Collaborator Author

Removed the incremental custom components

@btkcodedev
Copy link
Collaborator Author

Tested with builder
Streams are fetching data, hashed within manifest

@btkcodedev
Copy link
Collaborator Author

@DanyloGL
Copy link
Collaborator

@brianjlai is OOO, @natikgadzhi could you please take a look on the question above?

@btkcodedev btkcodedev self-assigned this Feb 20, 2025
@btkcodedev
Copy link
Collaborator Author

Resolved conflicts

@natikgadzhi
Copy link
Contributor

@brianjlai I've done the query change as you suggested.
I've checked segments again, but saw something weird in builder, If the is_client_side_incremental: true is applied, responses could not be extracted from builder, and the extracted_path is the same, but if is_client_side_incremental: true is removed, it extracts 9 records normally,
What should we do?

Have you tried testing in Builder both with and without input state? When running without state, it seems that is_client_side_incremental should not really make any effect because your input state is None, and therefore all received records should be emitted.

That together with regression tests not showing records in segments is suspicious. If there are only 2 workspaces using the stream, we can merge and roll forward if we have to at the end of the week, but we should debug this more.

Suspiciously, I don't see anything else that would stood out to me as different between segments and other streams that work as expected.

@btkcodedev
Copy link
Collaborator Author

I am also seeing no difference between segments and other streams
I'll look into it once more.
@brianjlai I could use some help here if you are available 🙏

@DanyloGL
Copy link
Collaborator

Stream segments didn't retrieve default segments due to creds issue. Start date was not correct to retrieve default segments. It strange that prod version retrieves them. Progressive Rollout can be started.

@DanyloGL
Copy link
Collaborator

DanyloGL commented Feb 26, 2025

/format-fix

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

🟦 Job completed successfully (no changes).

@DanyloGL
Copy link
Collaborator

DanyloGL commented Feb 26, 2025

/format-fix

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

🟦 Job completed successfully (no changes).

@DanyloGL DanyloGL merged commit 575eaf1 into master Feb 26, 2025
28 checks passed
@DanyloGL DanyloGL deleted the btkcodedev/intercomUpdateComponents branch February 26, 2025 14:36
@agarctfi agarctfi restored the btkcodedev/intercomUpdateComponents branch February 26, 2025 19:08
@agarctfi agarctfi deleted the btkcodedev/intercomUpdateComponents branch February 26, 2025 19:09
@DanyloGL
Copy link
Collaborator

Progressive Rollout failed. Rolled back, will investigate deeper.

@DanyloGL
Copy link
Collaborator

DanyloGL commented Feb 28, 2025

Many sync failures with new connector version. Need to investigate deeper what caused these failures.
cc @natikgadzhi

@btkcodedev
Copy link
Collaborator Author

Can i get access for that or a screenshot in slack would do, currently can't sign in with metabaseapp with my email

btkcodedev added a commit that referenced this pull request Mar 4, 2025
…mental sync components (#53187)

Co-authored-by: Octavia Squidington III <[email protected]>
Co-authored-by: Danylo Jablonski <[email protected]>
@DanyloGL
Copy link
Collaborator

@btkcodedev, hi! All failed connections had the same error for different affected streams:

2025-02-28 20:48:29 source ERROR Encountered an exception while reading stream contacts
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/abstract_source.py", line 152, in read
    yield from self._read_stream(
  File "/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/abstract_source.py", line 276, in _read_stream
    for record_data_or_message in record_iterator:
  File "/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/streams/core.py", line 196, in read
    for record_data_or_message in records:
  File "/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/declarative/declarative_stream.py", line 154, in read_records
    yield from self.retriever.read_records(self.get_json_schema(), stream_slice)  # type: ignore # records are of the correct type
  File "/usr/local/lib/python3.10/site-packages/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py", line 449, in read_records
    self.cursor.observe(_slice, current_record)
  File "/airbyte/integration_code/source_declarative_manifest/components.py", line 106, in observe
    if self.is_greater_than_or_equal(record, self._state):
  File "/airbyte/integration_code/source_declarative_manifest/components.py", line 131, in is_greater_than_or_equal
    return first_cursor_value > second_cursor_value
TypeError: '>' not supported between instances of 'int' and 'str'

@brianjlai
Copy link
Contributor

This is actually pretty interesting @DanyloGL this error message seems to be running on the outdated code. The stack trace you link refers to code from components.py that has been deleted as part of the deletion of the custom cursor code.

Here's the reference to line 131 which was since removed here: https://github.com/airbytehq/airbyte/pull/53187/files#diff-2f2cb95021afd772814a6f90bd568f9f840017b5c473c7b9959316cd8d526397L131

So I'm not sure how this would get triggered during the progressive rollout. Where did you find this stack trace because I suspect this was not running on @btkcodedev 's new intercom version because that code shouldn't exist anymore

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.

5 participants