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

Add timeout settings (default to 5 minutes) #15950

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
3 participants
@dadoonet
Member

dadoonet commented Jan 13, 2016

By default, azure does not timeout. This commit adds support for a timeout settings which defaults to 5 minutes.
It's a timeout per request not a global timeout for a snapshot request.

It can be defined globally, per account or both. Defaults to 5m.

cloud:
    azure:
        storage:
            timeout: 10s
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2
                timeout: 30s

In this example, timeout will be 10s for my_account1 and 30s for my_account2.

Backport of #15080 in 2.x branch.

Related to #14277.

(cherry picked from commit 96b3166)

@dadoonet

This comment has been minimized.

Member

dadoonet commented Jan 13, 2016

@imotov Assigning to you. It's a backport of #15080 in 2.x branch.

@dadoonet

This comment has been minimized.

Member

dadoonet commented Jan 18, 2016

Discussed with @imotov today.

As seen in #12567, this PR alone won't fix anything at it sounds although se define timeout settings, azure storage client does not throw any exception (see tests in Azure/azure-storage-java#71).
It could have been fixed in azure client 3.0.0 but the tests again don't seem to prove it.

At the very least, we won't merge this PR alone but only at the same time as we fix #15976.

Let's wait for the moment for Azure Storage Java SDK team's answer...

@dadoonet

This comment has been minimized.

Member

dadoonet commented Jan 19, 2016

@imotov You can move forward on this review as #16084 will fix the timeout issue we have seen so far.
Could you review it ASAP as it would be fantastic to have it in 2.0.4, 2.1.3 and 2.2.0?

cc @clintongormley

@dadoonet dadoonet added v2.2.0 and removed v2.2.1 labels Jan 19, 2016

@imotov

View changes

...cloud-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageServiceImpl.java Outdated
int timeout = (int) azureStorageSettings.getTimeout().getMillis();
client.getDefaultRequestOptions().setTimeoutIntervalInMs(timeout);
} catch (ClassCastException e) {
throw new IllegalArgumentException("Can not cast [" + azureStorageSettings.getTimeout() + "] to int.");

This comment has been minimized.

@imotov

imotov Jan 19, 2016

Member

Maybe change the error message to something less java-specific and more informative to include the name of the setting that we couldn't convert and saying that it cannot be longer than 2,147,483,647 ms?

@imotov

This comment has been minimized.

Member

imotov commented Jan 19, 2016

Left a minor comment. LGTM.

@imotov imotov removed their assignment Jan 19, 2016

Add timeout settings (default to 5 minutes)
By default, azure does not timeout. This commit adds support for a timeout settings which defaults to 5 minutes.
It's a timeout **per request** not a global timeout for a snapshot request.

It can be defined globally, per account or both. Defaults to `5m`.

```yml
cloud:
    azure:
        storage:
            timeout: 10s
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2
                timeout: 30s
```

In this example, timeout will be 10s for `my_account1` and 30s for `my_account2`.

Backport of #15080 in 2.x branch.

Related to #14277.

dadoonet added a commit that referenced this pull request Jan 19, 2016

Add timeout settings (default to 5 minutes)
By default, azure does not timeout. This commit adds support for a timeout settings which defaults to 5 minutes.
It's a timeout **per request** not a global timeout for a snapshot request.

It can be defined globally, per account or both. Defaults to `5m`.

```yml
cloud:
    azure:
        storage:
            timeout: 10s
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2
                timeout: 30s
```

In this example, timeout will be 10s for `my_account1` and 30s for `my_account2`.

Backport of #15080 (#15950) in 2.2 branch.

Related to #14277.

@dadoonet dadoonet merged commit 0a3f7b4 into elastic:2.x Jan 19, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@dadoonet dadoonet deleted the dadoonet:pr/15833-azure-timeout branch Jan 19, 2016

dadoonet added a commit that referenced this pull request Jan 19, 2016

dadoonet added a commit that referenced this pull request Jan 19, 2016

Add timeout settings (default to 5 minutes)
By default, azure does not timeout. This commit adds support for a timeout settings which defaults to 5 minutes.
It's a timeout **per request** not a global timeout for a snapshot request.

It can be defined globally, per account or both. Defaults to `5m`.

```yml
cloud:
    azure:
        storage:
            timeout: 10s
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2
                timeout: 30s
```

In this example, timeout will be 10s for `my_account1` and 30s for `my_account2`.

Backport of #15080 (#15950) in 2.0 branch.

Related to #14277.

dadoonet added a commit that referenced this pull request Jan 19, 2016

Add timeout settings (default to 5 minutes)
By default, azure does not timeout. This commit adds support for a timeout settings which defaults to 5 minutes.
It's a timeout **per request** not a global timeout for a snapshot request.

It can be defined globally, per account or both. Defaults to `5m`.

```yml
cloud:
    azure:
        storage:
            timeout: 10s
            my_account1:
                account: your_azure_storage_account1
                key: your_azure_storage_key1
                default: true
            my_account2:
                account: your_azure_storage_account2
                key: your_azure_storage_key2
                timeout: 30s
```

In this example, timeout will be 10s for `my_account1` and 30s for `my_account2`.

Backport of #15080 (#15950) in 2.1 branch.

Related to #14277.

@dadoonet dadoonet added v2.0.3 v2.1.3 v2.1.2 and removed v2.1.3 labels Jan 19, 2016

dadoonet added a commit to dadoonet/elasticsearch that referenced this pull request Jul 25, 2016

Remove TODO about Timeout in Azure
In elastic#15950 elastic#15080 elastic#16084 we added the support of TimeOut for Requests with a default client`setTimeoutIntervalInMs`.
So we can remove this useless todo which was added for only one method.

Closes elastic#18617.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment