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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Bugfix/644/listing makes pennsieve client fail #3464

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Oct 21, 2022

What do these changes do?

The connection with Pennsieve tends to fail after several calls to large datasets fail. This is an attempt to fix the problem.

  • remove dependency to pennsieve client, now authenticating directly using pennsieve recipes and boto3/raw client
  • remove dependency to pennsieve agent since we do not upload at the moment
  • upgrades to later best practices in datcore-adapter fastapi application
  • added 5 minutes caching to list files in a dataset
  • increased default timeout in storage/webserver clients since datasets in pennsieve tends to be large (a better fix would be using long running tasks and/or not listing everything inside a dataset as this is rather costly in performance)
  • now using cancel_on_disconnect decorator from servicelib instead of locally defined one
  • fixed loglevel parsing, added classic middlewares
  • added Makefile recipe to do real unit testing against pennsieve.io API

Bonus:

  • aiohttp now reports 504 Gateway Timeout error when the internal client times-out on a forwarded call to storage or another service instead of 500.

Related issue/s

related to ITISFoundation/osparc-issues#644
related to ITISFoundation/dockerfiles#72

How to test

Checklist

@sanderegg sanderegg added this to the Katherine Switzer milestone Oct 21, 2022
@sanderegg sanderegg self-assigned this Oct 21, 2022
@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #3464 (845cade) into master (8a9cb1e) will increase coverage by 2.0%.
The diff coverage is 87.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3464     +/-   ##
========================================
+ Coverage    83.3%   85.4%   +2.0%     
========================================
  Files         829     645    -184     
  Lines       35162   30044   -5118     
  Branches      739     552    -187     
========================================
- Hits        29321   25665   -3656     
+ Misses       5654    4231   -1423     
+ Partials      187     148     -39     
Flag Coverage 螖
integrationtests 67.0% <75.0%> (-0.8%) 猬囷笍
unittests 82.0% <87.4%> (+1.7%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...library/src/servicelib/aiohttp/rest_middlewares.py 74.1% <25.0%> (-1.9%) 猬囷笍
...service_storage/datcore_adapter/datcore_adapter.py 35.0% <25.0%> (-0.1%) 猬囷笍
...e_service_datcore_adapter/api/errors/http_error.py 53.8% <33.3%> (-4.5%) 猬囷笍
...rage/datcore_adapter/datcore_adapter_exceptions.py 69.2% <66.6%> (-0.8%) 猬囷笍
...rage/src/simcore_service_storage/utils_handlers.py 89.2% <66.6%> (-2.8%) 猬囷笍
...mcore_service_datcore_adapter/modules/pennsieve.py 92.9% <83.0%> (+0.1%) 猬嗭笍
...dels-library/src/models_library/app_diagnostics.py 100.0% <100.0%> (酶)
...ce_datcore_adapter/api/dependencies/application.py 100.0% <100.0%> (酶)
...vice_datcore_adapter/api/dependencies/pennsieve.py 100.0% <100.0%> (酶)
...imcore_service_datcore_adapter/api/module_setup.py 100.0% <100.0%> (酶)
... and 207 more

@sanderegg sanderegg force-pushed the bugfix/644/listing_makes_pennsieve_client_fail branch 2 times, most recently from 8b91fff to a3fed0e Compare October 24, 2022 11:15
@sanderegg sanderegg force-pushed the bugfix/644/listing_makes_pennsieve_client_fail branch from a3fed0e to 7c33f22 Compare October 24, 2022 14:52
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

馃憤

You already did some useful upgrades on this service. Can I persuade you to do one extra "maintenance" PR follow this one and

  • Add typecheck step (see unit-test-postgres-database for referece)
  • Do full update of reqs. (some constraints were added due to pennsieve lib)

I suggest them in a separate PR for clarity

status_code=status.HTTP_202_ACCEPTED,
)
@cancellable_request
async def upload_file_in_collection(
Copy link
Member

Choose a reason for hiding this comment

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

so these handlers are not anymore used? I do not see a replacement

Copy link
Member Author

Choose a reason for hiding this comment

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

no they are removed. and as a matter of fact they were never used.

@sanderegg
Copy link
Member Author

+1

You already did some useful upgrades on this service. Can I persuade you to do one extra "maintenance" PR follow this one and

  • Add typecheck step (see unit-test-postgres-database for referece)
  • Do full update of reqs. (some constraints were added due to pennsieve lib)

I suggest them in a separate PR for clarity

@pcrespov 馃憤

  1. requirements are already on the latest as far as I can tell
  2. added the typechecking...

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Nice.
Please find some questions below.

@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 2022

Kudos, SonarCloud Quality Gate passed!聽 聽 Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

None yet

3 participants