feat: Support config timeout for AlibabaCloud OSS requests #14856
Conversation
github-actions
bot
added
the
type/docs
| } | ||
|
|
||
| // NewOssObjectClient makes a new chunk.Client that writes chunks to OSS. | ||
| func NewOssObjectClient(_ context.Context, cfg OssConfig) (client.ObjectClient, error) { | ||
| client, err := oss.New(cfg.Endpoint, cfg.AccessKeyID, cfg.SecretAccessKey) | ||
| var options []oss.ClientOption | ||
| if cfg.ConnectionTimeoutSec > 0 || cfg.ReadWriteTimeoutSec > 0 { |
There was a problem hiding this comment.
looking at the code, setting ConnectionTimeoutSec to 0 means no timeout on connection. but setting ReadWriteTimeoutSec to 0 would fail the reads and writes immediately i think
should we pass these values as is and have a validation on ReadWriteTimeoutSec during config validation to make sure it is greater than 0?
There was a problem hiding this comment.
There was a problem hiding this comment.
Done! move value check to a separate validate function is great!
Thanks for your fast reply!
| @@ -52,11 +54,17 @@ func (cfg *OssConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { | |||
| f.StringVar(&cfg.Endpoint, prefix+"oss.endpoint", "", "oss Endpoint to connect to.") | |||
| f.StringVar(&cfg.AccessKeyID, prefix+"oss.access-key-id", "", "alibabacloud Access Key ID") | |||
| f.StringVar(&cfg.SecretAccessKey, prefix+"oss.secret-access-key", "", "alibabacloud Secret Access Key") | |||
| f.Int64Var(&cfg.ConnectionTimeoutSec, prefix+"oss.conn-timeout-sec", 5, "Connection timeout in seconds") | |||
There was a problem hiding this comment.
nit: would be nice if we could revert the defaults to the existing values. 30 and 60s
| } | ||
|
|
||
| func (cfg *OssConfig) Validate() error { | ||
| if len(cfg.Bucket) == 0 { |
There was a problem hiding this comment.
by default loki validates all provider configs, this would fail if someone is using a different provider. CI is failing as a result, can you remove the extra validations?
I would suggest only validating ReadWriteTimeoutSec to ensure it is gt 0 as a zero value would result in all reads and writes to fail immediately as a result of timeout. ConnectionTimeoutSec can be gt or eq to 0 as it supports indefinite waits
What this PR does / why we need it:
Which issue(s) this PR fixes:
The AlibabaCloud OSS Client have a default connection timeout value with 30 seconds and read-write timeout with 60 seconds, I think it's too long and we should support user to config it.
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR