Skip to content

Tdl 12607 add page size as a configurable property#73

Open
prijendev wants to merge 7 commits intoadhoc-crest-masterfrom
TDL-12607-add-page-size-as-a-configurable-property
Open

Tdl 12607 add page size as a configurable property#73
prijendev wants to merge 7 commits intoadhoc-crest-masterfrom
TDL-12607-add-page-size-as-a-configurable-property

Conversation

@prijendev
Copy link
Copy Markdown
Contributor

@prijendev prijendev commented Apr 21, 2022

Description of change

  • Added is a configurable pagination parameter page_size.
  • Added pagination test case.

QA steps

  • Give the page_size parameter in the config and verify that tap calls the API with the given pagination parameter.
  • Verify that if page_size parameter is not provided then tap consider default page_size 1000.

Risks

Rollback steps

  • revert this branch

to sync to generate the required reports.
"""
selected_streams = catalog.get_selected_streams(state)
# If page_size found in config then used it else use default page size.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# If page_size found in config then used it else use default page size.
# If page_size is found in config then use it, else use the default page size.

Copy link
Copy Markdown
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

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

This needs test coverage around the new configurable property:

  • Can the tap handle types that are not int?
  • What happens if I pass in a page size of zero?
  • Does the tap actually fall back to the default and perform functionally?

@prijendev
Copy link
Copy Markdown
Contributor Author

This needs test coverage around the new configurable property:

  • Can the tap handle types that are not int?
  • What happens if I pass in a page size of zero?
  • Does the tap actually fall back to the default and perform functionally?

We have updated the code to handle different values of the page_size parameter.
The tap passes the default value of page_size if page_size is 0, empty string, invalid string, negative int, negative float.
Tap converts valid string and float values to int.

@prijendev prijendev requested a review from kspeer825 June 20, 2022 14:27
primary_keys_page_1 = set(primary_keys_list_1)
primary_keys_page_2 = set(primary_keys_list_2)

# Verify by primary keys that data is unique for page
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you update this validation to ensure all pages are unique rather than the first 2, see example here https://github.com/singer-io/tap-mixpanel/blob/2cf3a86231da042f76f4e3ef9fa8c23600b6f593/tests/tap_tester/test_all_fields_pagination.py#L134-L153

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated pagination test case.

Comment on lines +69 to +70
# Revert back page size to 1000 for other test cases.
self.PAGE_SIZE = 1000
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is the page size changed here? And could you update this test to cover a larger page size than 1 so that we are validating multiple records per page?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have updated the page size to 2 for a pagination test case. Smaller page size takes a long time to complete the sync. So, here we are using a smaller page size just for the pagination test case, and for the rest of the test cases, we are reverting back it to 1000.

@prijendev prijendev requested a review from kspeer825 June 29, 2022 13:30
# @staticmethod
# def name():
# return "tap_tester_google_analytics_pagination_test"
# TODO https://stitchdata.atlassian.net/browse/SRCE-5084
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This seems to be an old link (old JIRA environment).
Is there a new JIRA associated with it, if not can we remove this ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cosimon @kspeer825 Should we remove such a Jira link from the tap-tester? We are not sure whether it is an old Jira link or not because it is not accessible to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants