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 Amazon Seller Partner: Add GET_SELLER_FEEDBACK_DATA report #8021

Conversation

lizdeika
Copy link
Contributor

@lizdeika lizdeika commented Nov 16, 2021

What

Adds GET_SELLER_FEEDBACK_DATA report to Amazon Seller Partner source connector

🚨 User Impact 🚨

None

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

@github-actions github-actions bot added the area/connectors Connector related issues label Nov 16, 2021
@harshithmullapudi
Copy link
Contributor

@lizdeika Hey can you move this PR out of Draft once you are done with changes. Thanks for your contribution.

@lizdeika lizdeika marked this pull request as ready for review November 19, 2021 17:13
@harshithmullapudi
Copy link
Contributor

@lizdeika hey thanks for the contribution. I had a question
Can we use the replication_start_date and instead of a new key and then fetch it till date if it is full refresh and use the state for incremental sync.

@lizdeika
Copy link
Contributor Author

@lizdeika hey thanks for the contribution. I had a question
Can we use the replication_start_date and instead of a new key and then fetch it till date if it is full refresh and use the state for incremental sync.

Hi,
replication_start_date(in this connector) maps to createdSince( in amazon documentation) which is for getting report itself that was created not earlier than createdSince.
data_start_time and data_end_time map to dataStartTime and dataEndTime. These are for the time period of a data in a report that we will be creating.
These 3 fields can not be reused between each other, they exist for different purposes
https://github.com/amzn/selling-partner-api-docs/blob/main/references/reports-api/reports_2021-06-30.md

@harshithmullapudi
Copy link
Contributor

@lizdeika thanks for sharing the info that was really helpful. Can you share across output for integrations test and unit tests ?

@lizdeika lizdeika marked this pull request as draft November 23, 2021 14:30
@lizdeika lizdeika marked this pull request as ready for review November 25, 2021 15:31
@lizdeika
Copy link
Contributor Author

@harshithmullapudi I have educated myself on incremental syncs and slicing
and rethought and remade the implementation as you suggested :)

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Nov 25, 2021
@lizdeika
Copy link
Contributor Author

lizdeika commented Nov 25, 2021

@harshithmullapudi any idea why data from raw table(data gets synced successfully) does not go to deduped _scd table when Sync mode is "Incremental | Deduped + history"



class SellerFeedbackReports(IncrementalReportsAmazonSPStream):
name = "GET_SELLER_FEEDBACK_DATA"
Copy link
Member

Choose a reason for hiding this comment

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

Add doc string as other streams.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

small comments thanks for the contribution

Comment on lines +7 to +24
"Date": {
"type": ["null", "string"],
"format": "date"
},
"Rating": {
"type": ["null", "number"]
},
"Comments": {
"type": ["null", "string"]
},
"Response": {
"type": ["null", "string"]
},
"Order ID": {
"type": ["null", "string"]
},
"Rater Email": {
"type": ["null", "string"]
Copy link
Member

Choose a reason for hiding this comment

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

Is it the name of this fields correct with the raw data/response from the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, fields are named exactly like this

from Crypto.Cipher import AES
from source_amazon_seller_partner.auth import AWSSignature

REPORTS_API_VERSION = "2020-09-04"
ORDERS_API_VERSION = "v0"
VENDORS_API_VERSION = "v1"

REPORTS_MAX_WAIT_SECONDS = 50
# 33min. taken from real world experience working with amazon seller partner reports
REPORTS_MAX_WAIT_SECONDS = 1980
Copy link
Member

Choose a reason for hiding this comment

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

This is based on your amount of data? Maybe transform this to a parameter where people can increase in necessity?

Copy link
Contributor Author

@lizdeika lizdeika Nov 29, 2021

Choose a reason for hiding this comment

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

This is based on experience working with various different report types. They usually are generated faster but 30 minutes was maximum we saw. In any case 50 seconds is way too little.
Parameter is a good idea. Can I do it in another small PR?

Copy link
Member

Choose a reason for hiding this comment

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

sure.

@lizdeika
Copy link
Contributor Author

lizdeika commented Dec 1, 2021

@marcosmarxm @harshithmullapudi any blocking stuff to get this in?

@harshithmullapudi harshithmullapudi merged commit d7ef91f into airbytehq:master Dec 2, 2021
@lizdeika lizdeika deleted the amazon-seller-partner-GET_SELLER_FEEDBACK_DATA branch December 3, 2021 07:34
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
…rbytehq#8021)

* Source Amazon Seller Partner: Add GET_SELLER_FEEDBACK_DATA report

* no field definition here

* add dataStartTime, dataEndTime

* fixes, version bumps

* revert this

* cleanup

* fixes

* real world wait

* fix typo

* incremental, transformer

* fix stream schema

* implement incremental with slicing

* move

* cleanup

* update configured catalog

* fix tests

* changelog

* doc

* fix doc

* fix configured catalog

* Field definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants