TDL-16353: Add request timeout by hpatel41 · Pull Request #36 · singer-io/tap-sftp

hpatel41 · 2021-11-16T10:27:26Z

Description of change

TDL-16353: Add request timeout

Use default timeout of 300 seconds if the user does not enter the preferred timeout value

Manual QA steps

Risks

Rollback steps

revert this branch

NevilParikh14 · 2021-11-19T06:07:16Z

tap_sftp/client.py

+            timeout_value = float(timeout)
+            # update the request timeout for the requests
+            self.request_timeout = timeout_value
+        else:


Can you please add the comment related to 0, "0", "" values for else part.

savan-chovatiya · 2021-11-26T11:19:49Z

tap_sftp/client.py

+            timeout_value = float(timeout)
+            # set the request timeout for the requests
+            # if value is 0,"0", "" or None then it will set default to default to 300.0 seconds if not passed in config.
+            self.request_timeout = timeout_value


Suggested change

timeout_value = float(timeout)

# set the request timeout for the requests

# if value is 0,"0", "" or None then it will set default to default to 300.0 seconds if not passed in config.

self.request_timeout = timeout_value

self.request_timeout = float(timeout)

namrata270998 · 2021-11-26T13:38:01Z

tap_sftp/client.py

        sorted_files = sorted(matching_files, key = lambda x: (x['last_modified']).timestamp())
        return sorted_files

+    @backoff.on_exception(backoff.expo,


@harshpatel4crest Add comments for this backoff

Added comment.

namrata270998 · 2021-11-26T13:39:17Z

tap_sftp/sync.py


    return records_streamed

+@backoff.on_exception(backoff.expo,


@harshpatel4crest add comments for this backoff

Added comment.

dbshah1212 · 2021-12-20T09:15:53Z

tap_sftp/client.py

                self.transport.connect(username= self.username, password = self.password, hostkey = None, pkey = None)
                self.sftp = paramiko.SFTPClient.from_transport(self.transport)
            self.__active_connection = True
+            # get 'socket'


Please elaborate more, why do we need to get a socket?

To set the timeout we need socket.

dbshah1212 · 2021-12-20T09:17:42Z

tests/unittests/test_timeout.py

+        Test case to verify the timeout value is set as expected
+    """
+
+    def test_timeout_value_not_passed_in_config(self):


Please add comments for each function, what it is testing.

Added comment for each test case function.

dbshah1212 · 2021-12-20T09:18:03Z

tap_sftp/client.py


    def close(self):
        if self.__active_connection:
+            # get socket


Please elaborate more, why do we need to get a socket?

To set the timeout we need socket.

karanpanchal-crest · 2022-01-03T14:28:00Z

tap_sftp/client.py

+            # get 'socket' to set the timeout
+            socket = self.sftp.get_channel()
+            # set request timeout to 'None' ie. default value
+            socket.settimeout(None)


@harshpatel4crest Why do you have to set the timeout manually? Why not using the same request_timeout parameter?

Here, before closing the connection, the timeout is set to None (Default value).

Please use the default value/configured value provided in the tap. Request timeout should never have None value

Removed the code as during the connection we are setting the desired timeout value.

karanpanchal-crest · 2022-01-03T14:47:08Z

tap_sftp/client.py


 class SFTPConnection():
-    def __init__(self, host, username, password=None, private_key_file=None, port=None):
+    def __init__(self, host, username, password=None, private_key_file=None, port=None, timeout=None):


@harshpatel4crest Why timeout = None? You can set it to REQUEST_TIMEOUT instead to denote that default value will be taken if not provided

Added REQUEST_TIMEOUT instead of None.

karanpanchal-crest · 2022-01-05T14:32:51Z

tap_sftp/client.py

                          private_key_file=config.get('private_key_file'),
-                          port=config.get('port'))
+                          port=config.get('port'),
+                          timeout=config.get('request_timeout'))


@harshpatel4crest Why have you used request_timeout from config directly? Can't we useself.request_timeout?

Here we are initializing the connection. Hence we are passing the timeout value from the config.

kspeer825

The image needs updated.

kspeer825 · 2022-01-07T12:52:08Z

.circleci/config.yml

            source /usr/local/share/virtualenvs/tap-sftp/bin/activate
-            pip install nose
-            nosetests tests/unittests/
+            pip install nose coverage


Please update the tap-tester image to the latest stitch-tap-tester

@kspeer825 When we are updating image we are getting ModuleNotFoundError: No module named 'paramiko' error. Please find the error reference

That dependency will need to be imported directly in the circleci config. The preferred method for this is to use a test group under extras_require in the setup.py. Then this will have to be installed via pip under the integration test step in the workflow definition. This was one of the breaking changes to tap-tester that resulted in the image actually - removing tap dependencies from the test framework repo.

kspeer825 · 2022-01-07T13:41:03Z

.circleci/config.yml

+            run-test --tap=tap-sftp \
+                     --target=target-stitch \
+                     --orchestrator=stitch-orchestrator \
+                     --email=harrison+sandboxtest@stitchdata.com \
+                     --password=$SANDBOX_PASSWORD \
+                     --token=$DEV_TOKEN \
+                     --client-id=50 \
+                     tests


something like pip install ../.[test] prior to the run-test command where the test group contains the same paramiko 2.6 dependency that is required by the tap.

Added the test group to install paramiko.

added timeout

9af65e7

hpatel41 marked this pull request as ready for review November 16, 2021 11:10

hpatel41 requested review from dbshah1212, karanpanchal-crest and savan-chovatiya November 16, 2021 11:12

NevilParikh14 reviewed Nov 19, 2021

View reviewed changes

harshpatel4_crest added 2 commits November 19, 2021 17:55

updated request timeout code

6710aca

added backoff for get_file_handle

cd362ab

savan-chovatiya reviewed Nov 26, 2021

View reviewed changes

namrata270998 reviewed Nov 26, 2021

View reviewed changes

resolve comments

4ff4e60

hpatel41 requested review from NevilParikh14, namrata270998 and savan-chovatiya November 29, 2021 06:52

namrata270998 approved these changes Dec 1, 2021

View reviewed changes

dbshah1212 reviewed Dec 20, 2021

View reviewed changes

resolve review comments

09b7af5

hpatel41 requested a review from dbshah1212 December 23, 2021 12:44

karanpanchal-crest suggested changes Jan 3, 2022

View reviewed changes

added REQUEST_TIMEOUT as default value instead of None

56fc68d

hpatel41 requested a review from karanpanchal-crest January 4, 2022 07:46

Merge branch 'master' into add-request-timeout

23e4c46

dbshah1212 approved these changes Jan 5, 2022

View reviewed changes

removed timeout none before closing connection

0fc4fc0

karanpanchal-crest reviewed Jan 5, 2022

View reviewed changes

karanpanchal-crest approved these changes Jan 5, 2022

View reviewed changes

umangagrawal-crest requested a review from KrisPersonal January 5, 2022 15:50

umangagrawal-crest requested a review from kspeer825 January 5, 2022 15:50

kspeer825 suggested changes Jan 7, 2022

View reviewed changes

harshpatel4_crest added 2 commits January 7, 2022 18:34

updated tap-tester image

f13381b

reverted image

445a403

kspeer825 reviewed Jan 7, 2022

View reviewed changes

added test group for integration tests

12d8986

hpatel41 requested a review from kspeer825 January 7, 2022 14:32

kspeer825 approved these changes Jan 10, 2022

View reviewed changes

KrisPersonal approved these changes Jan 10, 2022

View reviewed changes

changes for append to write in zip file test case

3d9c26a

KrisPersonal merged commit 935d7f9 into master Jan 11, 2022

KrisPersonal deleted the add-request-timeout branch January 11, 2022 16:59

Conversation

hpatel41 commented Nov 16, 2021

Description of change

Manual QA steps

Risks

Rollback steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namrata270998 Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hpatel41 Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kspeer825 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

namrata270998 Nov 26, 2021 •

edited

Loading

hpatel41 Jan 4, 2022 •

edited

Loading