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.

Sign up for GitHub

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

Jump to bottom

Tdl 20674 Reduce API calls to stripe v2.0.0 #150

Merged
merged 31 commits into from
Sep 22, 2022
Merged

Tdl 20674 Reduce API calls to stripe v2.0.0 #150

merged 31 commits into from
Sep 22, 2022

Conversation

prijendev
Copy link
Contributor

@prijendev prijendev commented Sep 13, 2022 •
edited by sgandhi1311
Loading

Description of change

  • Added support of config parameter event_date_date_window.
  • Default event_date_date_window is 7 days and maximum possible value of it is 30 days.
  • Make API call of event_updates for last 30 days only. If start_date or last saved bookmark value is before 30 days, then start the sync from the last 30 days only.
  • Write maximum of replication key value or sync_start_time - event_date_window as bookmark for event_updates.
  • Retry 429 error 7 times with exponential factor 2.

Manual QA steps

Risks

Rollback steps

  • revert this branch

@prijendev prijendev changed the title Tdl 20557 reduced api calls to stripe Tdl 20674 reduced api calls to stripe Sep 16, 2022
@prijendev prijendev marked this pull request as ready for review September 16, 2022 08:18
@prijendev prijendev changed the title Tdl 20674 reduced api calls to stripe Tdl 20674 Reduce API calls to stripe v2.0.0 Sep 16, 2022
dbshah1212
dbshah1212 reviewed Sep 16, 2022
View reviewed changes
tap_stripe/__init__.py Outdated Show resolved Hide resolved
dbshah1212
dbshah1212 reviewed Sep 16, 2022
View reviewed changes
tap_stripe/__init__.py Outdated Show resolved Hide resolved
dbshah1212
dbshah1212 reviewed Sep 16, 2022
View reviewed changes
tap_stripe/__init__.py Outdated
event_window_size = 30
LOGGER.warning("Using event window size of 30 days for %s stream.", stream_name)
events_date_window_size = int(60 * 60 * 24 * event_window_size) # event_window_size in seconds
sync_start_time = dt_to_epoch(utils.now()) - events_date_window_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sync_start_time = dt_to_epoch(utils.now()) - events_date_window_size
sync_start_time = dt_to_epoch(utils.now())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed events_date_window_size and moved it at bottom part of function for better readability.

dbshah1212
dbshah1212 reviewed Sep 16, 2022
View reviewed changes
tap_stripe/__init__.py Outdated
# Write bookmark for parent or child stream if it is selected
write_bookmark_for_event_updates(is_sub_stream, stream_name, sub_stream_name, max_created)

max_created = max(max_created, sync_start_time)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max_created = max(max_created, sync_start_time)
max_created = max(max_created, sync_start_time-events_date_window_size)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated logic to write the final bookmark value.

dbshah1212
dbshah1212 reviewed Sep 19, 2022
View reviewed changes
tap_stripe/__init__.py Outdated
date_window_start = max_created
date_window_end = max_created + date_window_size
# Start sync from 30 days before if bookmark/start_date is older than 30 days.
max_created = int(max(bookmark_value, (singer.utils.now() - timedelta(days=30)).timestamp()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max_created = int(max(bookmark_value, (singer.utils.now() - timedelta(days=30)).timestamp()))
max_created = int(max(bookmark_value, (sync_start_time - timedelta(days=30)).timestamp()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced singer.utils.now() with sync_start_time to remove minor diff of the time.

dbshah1212
dbshah1212 reviewed Sep 19, 2022
View reviewed changes
tap_stripe/__init__.py Outdated
# Write bookmark for parent or child stream if it is selected
write_bookmark_for_event_updates(is_sub_stream, stream_name, sub_stream_name, max_created)

max_created = max(max_created, sync_start_time - events_date_window_size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add code comment and reason behind this change.

dbshah1212
dbshah1212 reviewed Sep 19, 2022
View reviewed changes
tap_stripe/__init__.py Outdated Show resolved Hide resolved
dbshah1212
dbshah1212 reviewed Sep 19, 2022
View reviewed changes
tap_stripe/__init__.py Outdated Show resolved Hide resolved
dbshah1212
dbshah1212 reviewed Sep 19, 2022
View reviewed changes
tap_stripe/__init__.py Outdated Show resolved Hide resolved
dbshah1212
dbshah1212 reviewed Sep 20, 2022
View reviewed changes
tap_stripe/__init__.py Outdated
@@ -451,7 +452,12 @@ def reduce_foreign_keys(rec, stream_name):
rec['lines'][k] = [li.to_dict_recursive() for li in val]
return rec


# Retry 429 RateLimitError 5 times.
Copy link
Contributor

Choose a reason for hiding this comment

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

6 times.

Copy link
Contributor Author

@prijendev prijendev Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We have updated retry count to 7. Hence, updated message according to this.

RushiT0122
RushiT0122 reviewed Sep 20, 2022
View reviewed changes
tap_stripe/__init__.py
# Write the parent bookmark value only when the parent is selected
if not is_sub_stream:
singer.write_bookmark(Context.state,
stream_name + '_events',
Copy link
Contributor

Choose a reason for hiding this comment

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

We are already writing state in write_bookmark_for_event_updates() so we can remove this statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed write_state statements.

RushiT0122
RushiT0122 reviewed Sep 20, 2022
View reviewed changes
tap_stripe/__init__.py
max_created)
singer.write_state(Context.state)


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this statement and put at the last line of this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved write_state to common place.

RushiT0122
RushiT0122 requested changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Requested changes inline.

@sgandhi1311
Copy link
Member

As CircleCi is down, the following test scenarios are tested locally on dev-vm -

Unit tests -

test_auth_in_discovery
test_child_bookmarks
test_date_window_size
test_deleted_invoice_line_item
test_dict_to_stripe_object
test_get_and_write_bookmark
test_invoice_line_item_id
test_log_request_id
test_logger_for_events
test_lookback_evaluation
test_lookback_window
test_payout_events_object
test_rate_limit_error
test_recursive_to_dict
test_request_timeout
test_sync_event_updates

Integration Tests -

test_all_fields
test_all_test_runs
test_automatic_fields
test_automatic_payout_transactions
test_bookmarks
test_configurable_lookback_window
test_create_object
test_discovery
test_event_updates
test_full_replication
test_pagination
test_parent_child_independent
test_start_date

@sgandhi1311 sgandhi1311 merged commit 3c1b24b into master Sep 22, 2022
@dmosorast dmosorast deleted the TDL-20557-reduced-api-calls-to-stripe branch September 23, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbshah1212 dbshah1212 dbshah1212 approved these changes

@bryantgray bryantgray bryantgray approved these changes

@karanpanchal-crest karanpanchal-crest karanpanchal-crest approved these changes

@savan-chovatiya savan-chovatiya savan-chovatiya approved these changes

@RushiT0122 RushiT0122 RushiT0122 approved these changes

@KrisPersonal KrisPersonal Awaiting requested review from KrisPersonal

@cosimon cosimon Awaiting requested review from cosimon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@prijendev @sgandhi1311 @bryantgray @karanpanchal-crest @dbshah1212 @savan-chovatiya @RushiT0122