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 Marketo: New Stream Segmentation #23956
Conversation
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.
Some comments.
airbyte-integrations/connectors/source-marketo/source_marketo/schemas/segmentations.json
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-marketo/source_marketo/source.py
Outdated
Show resolved
Hide resolved
def path(self, **kwargs) -> str: | ||
return f"rest/asset/v1/segmentation/{self.segment_id}/segments.json" |
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.
Why are you using this endpoint and not the one to retrieve all segmentations?
https://developers.marketo.com/rest-api/endpoint-reference/asset-endpoint-reference/#!/Segments/getSegmentsForSegmentationUsingGET
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.
The issue stated to add the stream which returns list of segments inside target segmentation
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.
@YowanR this change adds a new spec field where users need to specify the segmentation they want to retrieve. If they need two or all it won't be possible. Marketo has the option to retrieve all segmentations and we could use that to be parent stream and do automatically retrieving all data. Any comments?
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.
I prefer that route as well because it makes it convenient as users would not need to search for the segmentIDs
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.
I'll make the changes to have it return the entire segmentation. Is that alright?
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.
No, I think this stream you created must retrieve the endpoint https://developers.marketo.com/rest-api/endpoint-reference/asset-endpoint-reference/#!/Segments/getSegmentationUsingGET and you must create another called Segments which uses the parent data (segmentation id) to retrieve the data for https://developers.marketo.com/rest-api/endpoint-reference/asset-endpoint-reference/#!/Segments/getSegmentsForSegmentationUsingGET
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.
So there won't be a segmentationId in the spec right? Also creating a stream which reads the segmentationId would then mean that it is returning the data of all the segments. Is that correct?
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.
You're correct. you must create the parent stream to retrieve all segmentations and other to read the segments. Take a look in https://docs.airbyte.com/connector-development/cdk-python/http-streams#nested-streams--caching
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.
@marcosmarxm are there any already implemented nested streams that you know of which I can refer to get a better understanding of it?
Hey thanks for the contribution and apologies for the delay! I'll be the DRI responsible for reviewing this and getting it across the finish line. We currently have a backlog of about a dozen PRs, but just wanted to let you know that this PR has been added to the queue and I'll be trying my best to get to this as soon as possible. Thanks for the contribution and for being patient :) |
Hello and thank you for adding it to the list :D |
/test connector=connectors/source-marketo
|
/test connector=connectors/source-marketo
|
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.
Thanks for the contribution @Alcadeus0 rigth now Marketo is timing out integration tests. I request the connector team to take a look. There isn't anything pending from your side.
/test connector=connectors/source-marketo
Build PassedTest summary info:
|
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! Minor comment for documentation.
@@ -106,6 +106,7 @@ If the 50,000 limit is too stringent, contact Marketo support for a quota increa | |||
|
|||
| Version | Date | Pull Request | Subject | | |||
|:---------|:-----------|:---------------------------------------------------------|:----------------------------------------------------------------------------------------------| | |||
| `1.1.0` | 2023-04-18 | [23956](https://github.com/airbytehq/airbyte/pull/23956) | Add `Segmentations` Stream | |
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.
We need add stream to the Supported Streams
section.
/publish connector=connectors/source-marketo
if you have connectors that successfully published but failed definition generation, follow step 4 here |
* Added Segmentation Stream * change endpoint to read all segments * remove unused params * add json schema header * bump connector version and update doc * remove f-string not used * fix schema and correct segmentations base stream class * remove incremental from int test segmenations config cat * fix segmentations params * add expected records * Update marketo.md * bump the metadata file * auto-bump connector version --------- Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com> Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
Fixes #17547
How
Added new Stream Segmentation which returns list of segments from target segmentation
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
No
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
Updating a connector
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described here