fix(source-greenhouse): remove custom cursors by darynaishchenko · Pull Request #54702 · airbytehq/airbyte

Daryna Ishchenko (darynaishchenko) · 2025-02-27T12:40:19Z

What

https://github.com/airbytehq/airbyte-internal-issues/issues/11527

How

Based on cdk changes in airbytehq/airbyte-python-cdk#369 we now able to use %_ms as milliseconds identifier.
Removed custom cursors and added incremental sync based on DatetimeBasedCursor.
State migrations from legacy to per partition were added to make state change non-breaking.

Review guide

User Impact

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2025-02-27T12:40:24Z

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	Mar 14, 2025 5:30pm

Fume OSS (fumeoss) · 2025-02-27T18:35:31Z

Summary

Removed custom cursors in the Greenhouse source integration and implemented incremental sync using DatetimeBasedCursor.
Introduced state migrations from legacy to per-partition storage to ensure non-breaking changes.
Updated the connector's version from 0.5.32 to 0.6.0 in the metadata and manifest files.
Modified the GreenhouseStateMigration class to handle partition key migrations correctly.
Updated documentation to reflect the changes made in the connector.

🧪 3 passed

🟢 Test Sub-stream State Migration for source-greenhouse

🟢 Test Legacy State Migration for Greenhouse Connector - Jira Issue #11527

🟢 Test Millisecond Precision Datetime Format for DatetimeBasedCursor Implementation

_{Do you want to test every PR like this? 👉 make Fume a regular reviewer at Airbyte}

Daryna Ishchenko (darynaishchenko) · 2025-02-28T11:41:44Z

Regression tests:

test_catalog_are_the_same[failed]

is_resumable now true(due to cdk update)

TestDataIntegrity.test_record_schema_match_with_state [failed]

Null fields are not in records(due to cdk update)

Record count mismatch

some inctemental streams have more records in target version, data could be added between reads, we should be fine with this.

Natik Gadzhi (natikgadzhi)

The code changes look good to me, granted I'm still not fully an expert in the area. Given the CDK change landed, I'd love to merge this, do a single regression test (if we haven't already) and start a progressive rollout.

The only ask I have is to bump pytest to 8, deps hygiene.

Daryna Ishchenko (@darynaishchenko) take this away, let's get this shipped.

airbyte-integrations/connectors/source-greenhouse/unit_tests/conftest.py

Natik Gadzhi (natikgadzhi) · 2025-03-07T00:32:15Z

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/run.py

-def run():
-    source = SourceGreenhouse()
-    launch(source, sys.argv[1:])
+def _get_source(args: List[str]):


Nit, non-blocking: I think I've seen conversations about our approach to run.py — this seems like boilerplate, feels off. Is there a reason we have to have this code here, as opposed to having this inside a generic source init?

Probably cc Maxime Carbonneau-Leclerc (@maxi297) and Brian Lai (@brianjlai)

Natik Gadzhi (@natikgadzhi) I agree that we should probably want to ideally have this generified (is that a word?) into the CDK implementation, but the issue i think when we first build this was that run.py isn't even a thing in the CDK itself. So we were left with a bit of a manual process.

And while this is annoying, ultimately all these individual run.py definitions should ultimately get removed as we move to manifest-only connectors where all of this is already addressed within source-declarative-manifest https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-declarative-manifest/source_declarative_manifest/run.py#L152-L160.

I think unless we find free cycles to make this easier in the CDK, I don't think this is a super high prioirty because it will ultimately be throwaway work once we treat everything as manifest-only.

airbyte-integrations/connectors/source-greenhouse/pyproject.toml

Brian Lai (brianjlai)

overall looks good. if you have a chance can you rebase this w/ the latest master and re-run regression tests and I can do a final approval

Brian Lai (brianjlai) · 2025-03-13T02:31:07Z

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/components.py

-    def is_greater_than_or_equal(self, first: Record, second: Record) -> bool:
+        LegacyToPerPartitionStateMigration migrates partition keys as string, while real type of id in greenhouse is integer,
+        which leads to partition mismatch.
+        To prevent this type casting for partition key was added.


thanks for adding the comment for why we needed the custom component

Can you explain to me a little more about why this is needed. I think the explanation mostly makes sense, but I noticed that in the previous state format, it was stored as a string being the key, followed by that partition's state value. Why didn't run into the state key string vs. greenhouse id string in the old state format when we were using strings?

Not strictly a blocker, but it just seems interesting that we now have to convert state to integers when strings used to work

It was working before because custom StreamSlicer overrides stream_slices method:
so it has right partition where parent primary key(id) is integer: here
and converts parent primary key to string to access the cursor value from state: here

that makes things more clear. I missed the part where the old custom component was casting. thank you

Brian Lai (brianjlai) · 2025-03-13T04:38:22Z

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/manifest.yaml

      name: "applications"
      path: "applications"
+      cursor_field: "applied_at"
+      cursor_request_option: "created_after"


small suggestion, instead of adding these two arbitrary parameters that we then need to account for using field_name: "{{ parameters.get('cursor_request_option', 'updated_after'), I would suggest that we instead just define a new:

incremental_sync: type: DatetimeBasedCursor ... start_time_option: type: RequestOption inject_into: request_parameter field_name: "created_after"

I think that reads more easily than the extra parameter injection because at first I thought it was named wrong as cursor_request_option. wdyt?

removed cursor_request_option and cursor_field from parameters. updated incremental_sync to use $ref to basic incremental_sync instead and overrided start_time_option and cursor_field

Brian Lai (brianjlai)

just a few small suggestions and tweaks, but after those are fixed, this should be ready to go. approved ✅

Brian Lai (brianjlai) · 2025-03-13T20:44:46Z

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/components.py

-    def is_greater_than_or_equal(self, first: Record, second: Record) -> bool:
+        LegacyToPerPartitionStateMigration migrates partition keys as string, while real type of id in greenhouse is integer,
+        which leads to partition mismatch.
+        To prevent this type casting for partition key was added.


that makes things more clear. I missed the part where the old custom component was casting. thank you

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/manifest.yaml

Brian Lai (brianjlai)

just a few small suggestions and tweaks, but after those are fixed, this should be ready to go. approved ✅

Brian Lai (brianjlai) · 2025-03-13T20:44:46Z

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/components.py

-    def is_greater_than_or_equal(self, first: Record, second: Record) -> bool:
+        LegacyToPerPartitionStateMigration migrates partition keys as string, while real type of id in greenhouse is integer,
+        which leads to partition mismatch.
+        To prevent this type casting for partition key was added.


that makes things more clear. I missed the part where the old custom component was casting. thank you

airbyte-integrations/connectors/source-greenhouse/source_greenhouse/manifest.yaml

Daryna Ishchenko (darynaishchenko) added 6 commits February 18, 2025 11:22

update airbyte-cdk to ^6

509f134

updated source.py and run.py

fce6712

removed custom cursors and added custom state migration

402b385

removed custom cursor usage from manifest

cb8cfce

updated unit tests

d371fe7

fix cursors for eeoc_stream

7664473

Daryna Ishchenko (darynaishchenko) self-assigned this Feb 27, 2025

Octavia Squidington III (octavia-squidington-iii) added area/connectors Connector related issues connectors/source/greenhouse labels Feb 27, 2025

vercel bot deployed to Preview February 27, 2025 12:42 View deployment

update to latest cdk, bump version

82618e6

Octavia Squidington III (octavia-squidington-iii) added the area/documentation Improvements or additions to documentation label Feb 27, 2025

vercel bot deployed to Preview February 27, 2025 15:53 View deployment

Daryna Ishchenko (darynaishchenko) requested a review from a team February 27, 2025 16:32

Daryna Ishchenko (darynaishchenko) requested review from Artem Inzhyyants (artem1205) and Anatolii Yatsuk (tolik0) March 5, 2025 16:54

Natik Gadzhi (natikgadzhi) approved these changes Mar 7, 2025

View reviewed changes

Brian Lai (brianjlai) reviewed Mar 13, 2025

View reviewed changes

Daryna Ishchenko (darynaishchenko) added 2 commits March 13, 2025 16:33

updated dependencies and removed conftest.py

f53a74b

removed cursor related parameters from stream definitions

65456ae

vercel bot deployed to Preview March 13, 2025 14:52 View deployment

Daryna Ishchenko (darynaishchenko) added 2 commits March 13, 2025 17:38

Merge branch 'master' into daryna/source-greenhouce/milliseconds-cursor

a546642

updated changelog

bb9bdb9

Daryna Ishchenko (darynaishchenko) requested a review from Brian Lai (brianjlai) March 13, 2025 15:40

vercel bot deployed to Preview March 13, 2025 15:46 View deployment

Brian Lai (brianjlai) approved these changes Mar 13, 2025

View reviewed changes

removed stream_slice usage, refactored start_time_option

cf7c54b

Brian Lai (brianjlai) approved these changes Mar 13, 2025

View reviewed changes

Daryna Ishchenko (darynaishchenko) and others added 2 commits March 14, 2025 12:19

removed stream_slice usage, refactored start_time_option

cf7c54b

Merge branch 'master' into daryna/source-greenhouce/milliseconds-cursor

6bea957

vercel bot deployed to Preview March 14, 2025 10:32 View deployment

set Progressive Rollout

fb4e711

vercel bot deployed to Preview March 14, 2025 17:30 View deployment

Daryna Ishchenko (darynaishchenko) merged commit 3b18ecd into master Mar 14, 2025
27 checks passed

Daryna Ishchenko (darynaishchenko) deleted the daryna/source-greenhouce/milliseconds-cursor branch March 14, 2025 17:47

Sven Pöche (Valgard) pushed a commit to mayflower/airbyte that referenced this pull request Mar 25, 2025

fix(source-greenhouse): remove custom cursors (airbytehq#54702)

42974e5

Conversation

Daryna Ishchenko (darynaishchenko) commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

Uh oh!

vercel bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fume OSS (fumeoss) commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Daryna Ishchenko (darynaishchenko) commented Feb 28, 2025

Regression tests:

test_catalog_are_the_same[failed]

TestDataIntegrity.test_record_schema_match_with_state [failed]

Record count mismatch

Uh oh!

Natik Gadzhi (natikgadzhi) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Brian Lai (brianjlai) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brian Lai (brianjlai) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Brian Lai (brianjlai) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Daryna Ishchenko (darynaishchenko) commented Feb 27, 2025 •

edited

Loading

vercel bot commented Feb 27, 2025 •

edited

Loading

Fume OSS (fumeoss) commented Feb 27, 2025 •

edited

Loading

Natik Gadzhi (natikgadzhi) left a comment •

edited

Loading