-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 Amplitude: added missing attrs in events schema, enabled default availability strategy #25842
🎉 Source Amplitude: added missing attrs in events schema, enabled default availability strategy #25842
Conversation
…ailability strategy
Before Merging a Connector Pull RequestWow! 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:
If the checklist is complete, but the CI check is failing,
|
/test connector=connectors/source-amplitude
Build PassedTest summary info:
|
@@ -68,10 +67,6 @@ def url_base(self) -> str: | |||
subdomain = "analytics.eu." if self.data_region == "EU Residency Server" else "" | |||
return f"https://{subdomain}amplitude.com/api/" | |||
|
|||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connector does have custom error handling.
But we needed to handle such errors during the whole process of read_records, not just at the begining of the sync.
…lability-strategy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
availability strategy was removed, because for this stream API may return 404 (see docs ) during read of first slice.
@artem1205 |
Yep, but this means that availability strategy is useless here, isn't it? |
I suppose it can handle another types of errors, what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Could you please also add the missing column declarations to the cohorts stream? Here are all of the missing columns from the past week (they look pretty much teh same last month)
This comes from the "please check the datadog dashboard" note here. I guess our test doesn't catch it because cohorts
is declared as an empty stream.
@@ -30,7 +30,6 @@ acceptance_tests: | |||
extra_fields: no | |||
exact_order: no | |||
extra_records: yes | |||
fail_on_extra_columns: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot 🥳
def availability_strategy(self) -> Optional["AvailabilityStrategy"]: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checked - I think @midavadim is right. The availability strategy's get_first_record_for_slice
calls stream.read_records
so the 404 error handling that returns empty records should work, and the AS should say "connected to stream, but got 0 records". So this should work in that case 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM given Ella's concern is addressed!
…t-and-avialability-strategy' into midavadim/24941-amplitude-fix-sat-and-avialability-strategy
…lability-strategy
I have added missing fields for cohort stream. |
@midavadim that's fine with me! If any of the types are wrong, that's on the documentation, not us. And we'll catch them in the schema validation errors. Thank you! ⭐ |
Then I think we can safely close #24480 as well :) |
/publish connector=connectors/source-amplitude
if you have connectors that successfully published but failed definition generation, follow step 4 here |
…lability-strategy
…lability-strategy
…ault availability strategy (airbytehq#25842) * Added missing attrs in events schema, updated SAT, enabled default availability strategy * updated connector version * added missing fields for cohort stream * auto-bump connector version --------- Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
added missing attrs in events schema,
enabled default avilability strategy
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user?
For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.
If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Actions
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.0.0.1
Dockerfile
has version0.0.1
README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog with an entry for the initial version. See changelog exampledocs/integrations/README.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Updating a connector
Community member or Airbyter
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Connector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes