genouest / biomaj-download

Download microservice for BioMAJ
GNU Affero General Public License v3.0
1 stars 7 forks source link

Refactor downloaders #7

Closed lburlot closed 4 years ago

lburlot commented 5 years ago

This PR refactors the downloaders in particular the download, match and list methods are more generic now (class specific behaviour is defined in submethods - following the Template Method pattern to be pedantic).

The change should be purely internal (the unit tests are the same) so there is no need to increase the version number.

Note that the PR adds a test which needs a local iRODS server. This test is disabled in CI (see 329b3da and 975db53).

osallou commented 5 years ago

Travis failed on flake8

duboism commented 5 years ago

flake8 errors are fixed.

duboism commented 5 years ago

You're right. We have move back the test of archive in FTPDownload._download so the download will be retried quickly in case of corrupted archive.

osallou commented 5 years ago

What is status on https support?

lburlot commented 5 years ago

Our apologies, we'll restore https and sftp protocols support in the next commit.

osallou commented 5 years ago

Maybe could be done via dedicated classes extending http / ftp? But would have effects on biomaj core (and biomaj?) to declare new protocol classes. Or keeping the set_protocol func as before (getting default protocol in class variable but overloading it in instance variable and using instance var afterward)

osallou commented 5 years ago

Looks ok for curldownload, but i will need more time for review again... (many updates)

osallou commented 5 years ago

Hi @lburlot @duboism, curl_download needs to define optional keep-alive to fix some use cases (optional) Would need something like:

        if os.environ.get("CURL_KEEPALIVE"):
            keepAlive = int(os.environ["CURL_KEEPALIVE"])
            curl.setopt(pycurl.TCP_KEEPALIVE, True)        
            curl.setopt(pycurl.TCP_KEEPIDLE, keepAlive * 2)
            curl.setopt(pycurl.TCP_KEEPINTVL, keepAlive)

as you refactor curl download, could you add this to your code?

duboism commented 5 years ago

Hi @lburlot @duboism, curl_download needs to define optional keep-alive to fix some use cases (optional) [...] as you refactor curl download, could you add this to your code?

Sure. Do you want to use an environment variable or a global/bank setting ?

osallou commented 5 years ago

Env variable is easier, else would need to add new var in messages.

duboism commented 4 years ago

Env variable is easier, else would need to add new var in messages.

As discussed during the hackathon, we have implemented this in another branch. See institut-de-genomique/biomaj-download@6957cc4cd0e307f8b1ec235ea4d22b8ab417a134.