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 new http transport params to storage drivers #4160
base: main
Are you sure you want to change the base?
Conversation
I'm wondering...if we're going to do this -- it seems reasonable -- we should figure out a way to do this across all storage drivers, now just the S3 driver 🤔 |
Thanks for the feedback @milosgajdos! I'm happy to try that out in this pr. I'll push an update in the next day or so. Thx |
0a266fd
to
e5df937
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on adding OpenTelemetry instrumentation! That's been on our roadmap for a long time, and is currently being worked on in #3996. In the interests of not conflicting with that PR or duplicating efforts in adding OpenTelemetry instrumentation, it would be best to keep this PR focused on adding HTTP transport parameters. Could you please back out the OpenTelemetry changes? We would appreciate your help in getting #3996 ready to merge, too.
disableKeepAlivesParam := false | ||
disableKeepAlives := parameters["disablekeepalives"] | ||
switch disableKeepAlives := disableKeepAlives.(type) { | ||
case string: | ||
b, err := strconv.ParseBool(disableKeepAlives) | ||
if err != nil { | ||
return nil, fmt.Errorf("the disablekeepalives parameter should be a boolean") | ||
} | ||
disableKeepAlivesParam = b | ||
case bool: | ||
disableKeepAlivesParam = disableKeepAlives | ||
case nil: | ||
// do nothing | ||
default: | ||
return nil, fmt.Errorf("the disablekeepalives parameter should be a boolean") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very repetitive. Given that the same validation rules apply to all the boolean parameters, how about extracting the common logic to a function or closure? E.g.:
boolParam := func(name string) (bool, error) { /* ... */ }
disableKeepAlives, err := boolParam("disablekeepalives")
if err != nil {
return nil, err
}
If you want to get really fancy, you could apply the panic-recover error handling pattern to elide having to write if err != nil
when parsing each parameter, e.g.:
if boolParam("disablekeepalives") {
/* ... */
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that kind of refactoring be a separate pr? I was trying to maintain the pattern in nearby code. I'm happy to do that here for gcs.
The azure driver's config handling does seem easier to work with than the more manual extraction gcs & aws drivers do. Any reason we shouldn't move toward that for gcs, and aws?
This now also needs a rebase. |
e5df937
to
b31c8d5
Compare
Thanks for the review! I removed the otel work from this pr. I'll try #3996 and provide any feedback on otel there. |
This enables the azure, gcs, and s3 storage driver users to disable keep alives `"disablekeepalives"`. Fixes distribution#4159 Signed-off-by: T Van Doren <108194999+tvdfly@users.noreply.github.com>
b31c8d5
to
d58e3ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to disable HTTP keep-alives in the storage driver clients? Nowhere in this PR's description nor in #4159 is that explained. Each feature and configuration knob we accept has ongoing maintenance costs. We need to make sure the benefits to you and to other users outweigh the cost before we can accept maintaining this option for the entire lifetime of distribution 3.x.
@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage. | |||
| `usedualstack` | no | Use AWS dual-stack API endpoints. | | |||
| `accelerate` | no | Enable S3 Transfer Acceleration. | | |||
| `objectacl` | no | The S3 Canned ACL for objects. The default value is "private". | | |||
| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usedualstack
is already documented, three rows up.
@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage. | |||
| `usedualstack` | no | Use AWS dual-stack API endpoints. | | |||
| `accelerate` | no | Enable S3 Transfer Acceleration. | | |||
| `objectacl` | no | The S3 Canned ACL for objects. The default value is "private". | | |||
| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. | | |||
| `multipartcombinesmallpart` | no | Combines small pending uploads with the ready part when `true`. The default is `true`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this option implemented? I don't see it anywhere else in the PR nor could I find any evidence in the code base that this was already implemented.
@@ -32,6 +33,9 @@ Amazon S3 or S3 compatible services for object storage. | |||
| `usedualstack` | no | Use AWS dual-stack API endpoints. | | |||
| `accelerate` | no | Enable S3 Transfer Acceleration. | | |||
| `objectacl` | no | The S3 Canned ACL for objects. The default value is "private". | | |||
| `usedualstack` | no | Use AWS S3 dual-stack endpoints, which support both IPv6 and IPv4, when set to `true`. The default is `false`. | | |||
| `multipartcombinesmallpart` | no | Combines small pending uploads with the ready part when `true`. The default is `true`. | | |||
| `accelerate` | no | Use AWS S3 Transfer Acceleration endpoints when set to `true`. The default is `false`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accelerate
is already documented, three rows up.
This enables s3 storage driver users to disable keep alives
"disablekeepalives"
and disable idle connections"disableidleconnections"
.Fixes #4159