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

Feature/retry config #5174

Merged
merged 11 commits into from May 28, 2019

Conversation

Projects
None yet
3 participants
@Minimonium
Copy link
Contributor

commented May 19, 2019

Changelog: Feature: Implement conan.conf retry and retry-wait and CONAN_RETRY and CONAN_RETRY_WAIT to configure all retries for all transfers, including upload, download, and tools.download().
Docs: conan-io/docs#1295

Closes #1215

The feature to pull up download/upload retry and retry_wait configuration to conan.conf and environment variables CONAN_RETRY and CONAN_RETRY_WAIT.

By using them as config variables the user can customize the Conan network behaviour specific to the machine in a unified manner instead of defining the flags in each related command.
Parameters of the upload command and the tools.download force override the default values for customizable behaviour.

It requires to change the default values of parameters of the upload command to None, so we'd fall back to the configuration values.
Also, because of the unified value system default values are now consistent across the API. Because of that, it's required to change old tests since messages on the default behaviour on retries has changed.

The problem of pushing them via an API for the general download (and related) commands is that we'd need to propagate them all across the codebase:

uploader_downloader [Downloader]
rest_client_v1, rest_client_v2, rest_client_common? rest_client, net (default is fine)
auth_manager
remote_manager
installer [BinaryInstaller], cmd/download, source [complete_recipe_sources]
manager [install], graph_manager_base [build_graph?]
conan_api [workspace_install, install, install_reference, download]      
command [install, workplace, download]

And it would clutter the code with forwarded parameters which is not really worth it.

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Consider a different name for retry and retry_wait though

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I like the idea, but you are changing the default retry attempts, and that is breaking the behavior. Changing that shouldn't be needed, right?

@memsharded
Copy link
Contributor

left a comment

I'd prefer another approach for implementing this, what do you think @lasote

@@ -254,6 +255,8 @@ def print_progress(output, units, progress=""):


def call_with_retry(out, retry, retry_wait, method, *args, **kwargs):
if retry is None:

This comment has been minimized.

Copy link
@memsharded

memsharded May 21, 2019

Contributor

I'd prefer a different approach.
As there are arguments existing, use them, and if another value is to be used from the conan.conf, lets inject it much earlier in the call stack, as soon as possible.
In other words, better to have it coming always from argument than having to read another env-var here.

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

I like the idea, but you are changing the default retry attempts, and that is breaking the behavior. Changing that shouldn't be needed, right?

It's needed to detect when the value is forced and when the value is the defaulted one. I agree though that it's better to preserve existing defaults. Will see how to pull extraction up.

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

So, I've pulled up the fallback to the config values for the upload in conan_api because config values are applied only from that level, but left fall back values in uploader_downloader to keep the old behaviour (I left it in for non-obvious calls to preserve the old behaviour with old defaults).
The only place where I'm not sure it makes sense to have default ones is in conans/client/cmd/uploader.py (and we don't have tests to check for that one):
https://github.com/conan-io/conan/pull/5174/files#diff-ad1674306ad35622cdaa15f2eca15c81

Also, I have a problem with the tools.download. Since it makes sense to apply the config to everything, but the call here isn't wrapped by config environment variables, so how should one provide the context?
https://github.com/conan-io/conan/blob/develop/conans/client/tools/net.py#L59

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

I guess something like #5204? For the tools part.

@memsharded
Copy link
Contributor

left a comment

Please have a look to those minor comments, and I think it could be merged after that.

Show resolved Hide resolved conans/client/conf/__init__.py Outdated
Show resolved Hide resolved conans/client/conf/__init__.py Outdated
Show resolved Hide resolved conans/client/command.py Outdated

Minimonium added some commits May 27, 2019

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Note my comment about tools.download.

@Minimonium

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Hi @Minimonium

Each conan_api call is wrapped with:

with tools.environment_append(api._cache.config.env_vars):
                # Patch the globals in tools
                return f(*args, **kwargs)

So I'd say tools.download usage of env-vars will be fine.

@lasote lasote added this to the 1.16 milestone May 28, 2019

@memsharded memsharded merged commit 4e78595 into conan-io:develop May 28, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@memsharded

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Merged, will be in next 1.16, many thanks!

Check the first 2 lines I added in your first comment:

Changelog: Feature: Implement conan.conf retry and retry-wait and CONAN_RETRY and CONAN_RETRY_WAIT to configure all retries for all transfers, including upload, download, and tools.download().
Docs: conan-io/docs#1295

They are used to automatically generated the changelog each release :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.