Tdl 16328 implement request timeout #38

namrata270998 · 2021-11-16T13:05:40Z

Description of change
Added request timeout for SDK library functions with default timeout 300 seconds

Manual QA steps

The request should backoff 5 times in case it is timed out.
Error should be thrown in case of retries not successful after 5 trials
Checked the request timeout works for each stream and backoff

Risks

Rollback steps

revert this branch

savan-chovatiya · 2021-11-26T10:33:28Z

tap_dynamodb/dynamodb.py

+    # if request_timeout is other than 0,"0" or "" then use request_timeout
+    request_timeout = config.get('request_timeout')
+    if request_timeout and float(request_timeout):
+        request_timeout = float(request_timeout)
+    else: # If value is 0,"0" or "" then set default to 300 seconds.
+        request_timeout = REQUEST_TIMEOUT


@namrata270998, Create a separate function to calculate request timeout based on provided config as we are calculating the same thing in multiple functions.

savan-chovatiya · 2021-11-26T10:35:10Z

tap_dynamodb/sync_strategies/log_based.py

+@backoff.on_exception(backoff.expo,
+                          (ReadTimeoutError, ConnectTimeoutError),
+                          max_tries=5,
+                          factor=2)


Suggested change

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

savan-chovatiya · 2021-11-26T10:35:33Z

tap_dynamodb/sync_strategies/log_based.py

+@backoff.on_exception(backoff.expo,
+                          (ReadTimeoutError, ConnectTimeoutError),
+                          max_tries=5,
+                          factor=2)


Suggested change

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

savan-chovatiya · 2021-11-26T10:35:49Z

tap_dynamodb/sync_strategies/full_table.py

+@backoff.on_exception(backoff.expo,
+                          (ReadTimeoutError, ConnectTimeoutError),
+                          max_tries=5,
+                          factor=2)


Suggested change

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

@backoff.on_exception(backoff.expo,

(ReadTimeoutError, ConnectTimeoutError),

max_tries=5,

factor=2)

savan-chovatiya · 2021-11-26T10:44:06Z

.circleci/config.yml

-            make lint
+            pylint tap_dynamodb --disable 'duplicate-code,missing-module-docstring,missing-class-docstring,too-few-public-methods,line-too-long,fixme,missing-function-docstring,raise-missing-from,consider-using-f-string,too-many-locals,invalid-name,too-many-arguments'


@namrata270998 Do we need this change for any specific reason?.

reverted the change

karanpanchal-crest · 2022-01-06T13:55:29Z

tap_dynamodb/sync_strategies/full_table.py

@@ -35,7 +37,11 @@ def scan_table(table_name, projection, last_evaluated_key, config):

        has_more = result.get('LastEvaluatedKey', False)

-
+# Backoff for both ReadTimeout and ConnectTimeout error for 5 times


@namrata270998 Please add this bckoff expression above the scan_table function instead. Update the test cases accordingly.

@karanpanchal-crest The function scan_table contains a yield. Hence the backoff won't work on that function
Please refer

tap-dynamodb/tap_dynamodb/sync_strategies/full_table.py

Line 33 in 68439cf

yield result

for reference

karanpanchal-crest · 2022-01-06T13:56:55Z

tests/unittests/test_request_timeout.py

+from unittest import mock
+from unittest.mock import Mock
+from unittest.case import TestCase
+from botocore.exceptions import ClientError, ConnectTimeoutError, ReadTimeoutError


Remove ClientError if not used

karanpanchal-crest · 2022-01-06T13:57:46Z

Makefile

@@ -4,6 +4,6 @@ lint-tests:
 	pylint tests -d broad-except,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,duplicate-code,no-name-in-module,import-error,consider-using-f-string

 lint-code:
-	pylint tap_dynamodb -d broad-except,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,raise-missing-from,consider-using-f-string
+	pylint tap_dynamodb -d broad-except,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,raise-missing-from,consider-using-f-string,duplicate-code


Remove the duplicate-code from the global level. Instead handle it where needed

kspeer825

The tests look great. One improvement I see would be using variables on repeated values in those unittests to ensure typo's cannot hurt us.

kspeer825 · 2022-01-12T13:56:38Z

.circleci/config.yml

@@ -19,6 +19,17 @@ jobs:
          command: |
            source /usr/local/share/virtualenvs/tap-dynamodb/bin/activate
            make lint
+      - run:


Please update this file in line with the changes required on PR #37 and #35

@kspeer825 Updated this file as requested

kspeer825 · 2022-01-12T13:57:25Z

tests/unittests/test_request_timeout.py

+        """
+        Check whether the request backoffs properly for discover_streams for 5 times in case of ReadTimeoutError error.
+        """
+        with self.assertRaises(ReadTimeoutError):


I like this pattern

kspeer825 · 2022-01-12T14:01:50Z

tests/unittests/test_request_timeout.py

+        config = {"region_name": "dummy_region", "request_timeout": "100"}
+        dynamodb.get_client(config)
+        mock_config.assert_called_with(connect_timeout=100, read_timeout=100)


The only thing I can think to change is it would be better to set the default timeout value as a reusable variable for expectations. Also repeated values could be replaced with variables as well within each test. Just a suggestion though. I think they are readable and maintainable as-is, which is the important part.

@kspeer825 updated the code and replaced values with reusable variables.

KrisPersonal · 2022-01-12T16:42:35Z

tap_dynamodb/dynamodb.py


 def get_stream_client(config):
+    # get the request_timeout


What is the difference between get_stream_client and get_client function?

@KrisPersonal Amazon DynamoDB Streams provides API actions for accessing streams and processing stream records, which can be done through get_stream_client. Whereas, the get_client returns a low-level client representing Amazon DynamoDB

kspeer825

Just a couple config changes. Tests look good.

kspeer825 · 2022-01-24T19:26:34Z

.circleci/config.yml

+      - store_test_results:
+          path: test_output/report.xml
+      - store_artifacts:
+          path: htmlcov
      - run:
          name: 'Tap Tester'
          command: |
            aws s3 cp s3://com-stitchdata-dev-deployment-assets/environments/tap-tester/sandbox tap-tester.env


These environment variables are not the up-to-date set. Please use tap-tester-sandbox over sandbox.

Update to tap-tester-sandbox env variables set

kspeer825 · 2022-01-24T19:48:31Z

.circleci/config.yml

-                     tests
+            run-test --tap=tap-dynamodb tests
+      - slack/notify-on-failure:
+          only_for_branches: master
 workflows:
  version: 2
  commit:


Can you add the tap-tester-user context as well.

Added the tap-tester-user context

* Tdl 16328 implement request timeout (#38) * added request timeout * added test cases for the backoff * resolved pylint errors * updated pylint * fixd pylint * updated config.yml * resolved comments * resolved comments * resolved pylint errors * resolved comments * resolved comments * resolved comments * resolved slack-on-notify failure * removed scenarios * added tap-tester-user in config * TDL-16291 added code comments (#36) * added code comments * fixed typo * TDL-13692: Missing parent objects from projection expression causes key error and TDL-16140: Fix key error and index error while applying projection expression (#35) * added code change for handling key error and index error * resolve pylint error * updated the unittests * updated config.yml to run unittest to run on CCi * added unittests * updated the code to handle child dict data * added comments with example for projection * updated config.yml file and resolved PR review comments * resolve slack failure error in config file * updated version 2 to 2.1 * remoed SCENARIO from the test cases * resolved comments * reverted changes * added tap-tester-venv, tap_tester_sandbox and check for verifying that we replicated records Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * Tdl 15171 implement expression attributes (#37) * Initial commit for expression attributes * Updated circleci and full_table.py * resolved pylint error * Updated circleci * Updated unittest case path in circleci * Updated expression attribute * resolved pylint error * Updated prepare exprssion logic * Handled empty projection * added logic for nested expressions * added in log_based test and comments * added code comments * Updated comments * Resolved review comments * added comment * resolved comments * resolved pylint errors * updated config.yml and a typo * resolved slack-on-notify failure * removed scenarios * Changed the expressionattribute logic as suggested * resolved pylint error * fixed typo * added test case for '.' in fields * added testcase for prepare_projection() * resolved comments * added check for empty expression * handled corner case * resolved comments and updated unittests * removed unused import * resolved comments * fixed pylint failure * updated comment * changed the name of catalog's expression to expression-attributes * added tap-tester-user to the config, updated to nosetests * updated to nosetests * Changed expression-attributes to expression-attribute * Changed expression-attributes to expression-attribute * added tests for primary and replication key reserved words * renamed the file * added funciton comments * fixed pylint * changed the parameter name to attribute * changed the attribute name in new files added * expression-attribute -> expression-attributes in full table * test fixes for pk and hash key as expression-attributes * expression-attribute -> expression-attributes in one more test * fix log sync ref and log projection test name * Changed from attribute to attributes Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: KrishnanG <kgurusamy@talend.com> Co-authored-by: kspeer <kspeer@stitchdata.com> Co-authored-by: Harsh <80324346+harshpatel4crest@users.noreply.github.com> Co-authored-by: Prijen Khokhani <88327452+prijendev@users.noreply.github.com> Co-authored-by: KrishnanG <kgurusamy@talend.com> Co-authored-by: kspeer <kspeer@stitchdata.com>

namrata270998 added 6 commits November 16, 2021 05:50

added request timeout

2405054

added test cases for the backoff

1392579

resolved pylint errors

3a9c8fc

updated pylint

e658b3d

fixd pylint

f7977e2

updated config.yml

4ab905a

namrata270998 requested a review from dbshah1212 November 16, 2021 14:09

savan-chovatiya reviewed Nov 26, 2021

View reviewed changes

namrata270998 added 3 commits November 26, 2021 12:26

resolved comments

2d6145e

resolved comments

b301a06

resolved pylint errors

68439cf

namrata270998 requested a review from savan-chovatiya November 26, 2021 12:36

namrata270998 requested a review from karanpanchal-crest January 4, 2022 04:51

dbshah1212 approved these changes Jan 5, 2022

View reviewed changes

karanpanchal-crest suggested changes Jan 6, 2022

View reviewed changes

namrata270998 added 2 commits January 7, 2022 15:30

resolved comments

30e21ad

resolved comments

def4770

namrata270998 requested a review from karanpanchal-crest January 7, 2022 10:20

karanpanchal-crest approved these changes Jan 10, 2022

View reviewed changes

namrata270998 requested review from KrisPersonal, kspeer825 and manand31 January 11, 2022 06:27

kspeer825 suggested changes Jan 12, 2022

View reviewed changes

KrisPersonal reviewed Jan 12, 2022

View reviewed changes

resolved comments

45b9b80

namrata270998 added 2 commits January 13, 2022 15:05

resolved slack-on-notify failure

3f42e0f

removed scenarios

764bae6

namrata270998 requested a review from kspeer825 January 17, 2022 05:56

kspeer825 suggested changes Jan 24, 2022

View reviewed changes

added tap-tester-user in config

0e85aee

namrata270998 requested a review from kspeer825 January 27, 2022 06:37

kspeer825 approved these changes Jan 27, 2022

View reviewed changes

namrata270998 merged commit 1665782 into TDL-16126-Crest-Master Feb 11, 2022

namrata270998 mentioned this pull request Feb 14, 2022

Tdl 16126 crest master #39

Merged

luandy64 deleted the TDL-16328-implement-request-timeout branch July 18, 2023 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tdl 16328 implement request timeout #38

Tdl 16328 implement request timeout #38

namrata270998 commented Nov 16, 2021

savan-chovatiya Nov 26, 2021

namrata270998 Nov 26, 2021

savan-chovatiya Nov 26, 2021

namrata270998 Nov 26, 2021

savan-chovatiya Nov 26, 2021

namrata270998 Nov 26, 2021

savan-chovatiya Nov 26, 2021

namrata270998 Nov 26, 2021

savan-chovatiya Nov 26, 2021

namrata270998 Nov 26, 2021

karanpanchal-crest Jan 6, 2022

namrata270998 Jan 7, 2022 •

edited

Loading

karanpanchal-crest Jan 6, 2022

namrata270998 Jan 11, 2022

karanpanchal-crest Jan 6, 2022

namrata270998 Jan 7, 2022

kspeer825 left a comment

kspeer825 Jan 12, 2022

namrata270998 Jan 17, 2022

kspeer825 Jan 12, 2022

kspeer825 Jan 12, 2022

namrata270998 Jan 17, 2022

KrisPersonal Jan 12, 2022

namrata270998 Jan 17, 2022

kspeer825 left a comment

kspeer825 Jan 24, 2022

namrata270998 Jan 27, 2022

kspeer825 Jan 24, 2022

namrata270998 Jan 27, 2022

		make lint
		pylint tap_dynamodb --disable 'duplicate-code,missing-module-docstring,missing-class-docstring,too-few-public-methods,line-too-long,fixme,missing-function-docstring,raise-missing-from,consider-using-f-string,too-many-locals,invalid-name,too-many-arguments'

		@@ -35,7 +37,11 @@ def scan_table(table_name, projection, last_evaluated_key, config):

		has_more = result.get('LastEvaluatedKey', False)


		# Backoff for both ReadTimeout and ConnectTimeout error for 5 times

Tdl 16328 implement request timeout #38

Tdl 16328 implement request timeout #38

Conversation

namrata270998 commented Nov 16, 2021

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namrata270998 Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namrata270998 Jan 7, 2022 •

edited

Loading