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

fix: connection dropped for refresh-data [TCTC-6839] #600

Merged
merged 10 commits into from
Oct 13, 2023

Conversation

Sanix-Darker
Copy link
Contributor

@Sanix-Darker Sanix-Darker commented Oct 3, 2023

WHAT

connection dropped on remote file (SFTP).
This is not a "new issue", my proposition on this PR is to increase the timeout, the retry and
suppress the .close() error and let see on live prod how it behave.

  • add more logging information.

HOW

  • feat: increase timeout values and the retry amount
  • fix: suppress the .close error for None type ?
  • chore: add more logs while accessing the remote SFTP file.

@Sanix-Darker Sanix-Darker self-assigned this Oct 3, 2023
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (7ecd199) 100.00% compared to head (006ebab) 100.00%.
Report is 1 commits behind head on release/v0.9.

Additional details and impacted files
@@              Coverage Diff               @@
##           release/v0.9      #600   +/-   ##
==============================================
  Coverage        100.00%   100.00%           
==============================================
  Files                19        19           
  Lines               839       845    +6     
==============================================
+ Hits                839       845    +6     
Files Coverage Δ
peakina/io/ftp/ftp_utils.py 100.00% <100.00%> (ø)
peakina/readers/excel.py 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sanix-Darker Sanix-Darker added bug Something isn't working need review labels Oct 3, 2023
@Sanix-Darker Sanix-Darker marked this pull request as ready for review October 3, 2023 14:46
Copy link
Contributor

@WilliamGorge WilliamGorge left a comment

Choose a reason for hiding this comment

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

Merci

peakina/io/ftp/ftp_utils.py Outdated Show resolved Hide resolved
@WilliamGorge
Copy link
Contributor

WilliamGorge commented Oct 3, 2023

on merge pas tant qu'on n'est pas sur que ça fix le souci du client

@Sanix-Darker Sanix-Darker force-pushed the fix/connection-dropped branch 2 times, most recently from a377fa6 to 4fd67a8 Compare October 6, 2023 13:34
@Sanix-Darker Sanix-Darker marked this pull request as draft October 7, 2023 08:42
@Sanix-Darker Sanix-Darker force-pushed the fix/connection-dropped branch 3 times, most recently from 1e0ba0c to 36f76d4 Compare October 9, 2023 08:48
Comment on lines 203 to 213
log.warning(f"File '{file_path}' not available inside : '{dir_path}' !")

parsed_url = urllib.parse.urlsplit(url)
path_without_file = parsed_url.path.rsplit("/", 1)[0]
modified_url = urllib.parse.urlunsplit(
(parsed_url.scheme, parsed_url.netloc, path_without_file, "", "")
)

files_available = ", ".join(ftp_listdir(modified_url)[:15])
# we list only 15 as maximum
log.warning(f"List of files : ({files_available})")
Copy link
Contributor

Choose a reason for hiding this comment

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

ça serait plus simple et lisible d'avoir qu'un seul log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates here : a72fc9c

@Sanix-Darker Sanix-Darker force-pushed the fix/connection-dropped branch from 36f76d4 to d8118be Compare October 13, 2023 06:48
but for each ftp-client it was defined at 3s,
Setting it to 10s is too small when we have a long list of files that could takes too much time to resolve,
the value should be increased, in this commit, am tripling the default fomr FTPS class that was at 60s to 180s.

Later on, we should find a way to set a FF that will forward that
extremly HUGE value to the lib itself.
@Sanix-Darker Sanix-Darker force-pushed the fix/connection-dropped branch from 8fa1ca9 to e874a86 Compare October 13, 2023 12:48
@Sanix-Darker Sanix-Darker marked this pull request as ready for review October 13, 2023 13:03
Comment on lines +20 to +21
_DEFAULT_MAX_TIMEOUT_SECONDS = 30
_DEFAULT_MAX_RETRY = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

tu augmente quand même le timeout et les retry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

le retry etait 4
le timeout etait 3

Tu suggeres que je remettes ces valeurs ?
Je me disais que ces valeurs ne sont pas deconantes non plus.

et si tu regardes la valeur du timeout par defaut de la calsse FTP, il etait a 60s : https://github.com/toucantoco/peakina/blob/main/peakina/io/ftp/ftp_utils.py#L27

ssh_client.close()
# In cae of Exception, we don't want to raise it
with suppress(AttributeError):
ssh_client.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

je loggerait quand même le cas ou on arrive pas à close du coup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants