#351 Add support for configuring uploaders and upload queue size. #359
Conversation
| } | ||
| }(remote, baseURL, accessLogger, errorLogger) | ||
| } | ||
| errorLogger cache.Logger, numUploaders, maxQueuedUploads int) cache.Proxy { |
There was a problem hiding this comment.
Have you considered passing in config.HTTPBackendConfig struct here? Instead of individual baseUrl, numUploaders, maxQueuedUploads parameters. In order to allow adding additional parameters more transparently in the future.
Example: if I would create an internal patch for the HTTP proxy, that is not accepted upstream. Such patch would be easier to maintain, with less merge conflicts, if it’s configuration parameters would be more hidden from main.go inside the struct.
There was a problem hiding this comment.
I prefer to keep configuration code strictly in the main package and treat the rest of the code like a library. The main package gathers configs, chooses interface implementations and ties them all together. The idea is to keep dependencies between packages very, very light.
Have you considered creating your own main package for bazel-remote? This is our approach for hooking into our internal services (like logging, configuration management). It works pretty well, but I think there is a lot we can do to allow users to drop dependencies they don't actually need
There was a problem hiding this comment.
It's very interesting to hear about your local forks. Please do open issues for anything that you think might be worth upstreaming.
I prefer to have individual arguments here- there's less chance of breakage due to changes to raw arguments, compared to a struct.
config/config.go
Outdated
|
|
||
| // ProxyUploadersConfig stores the configuration for uploaders and queue size in a Proxy backend. | ||
| type ProxyUploadersConfig struct { | ||
| Num int `yaml:"num_uploaders"` |
There was a problem hiding this comment.
"NumUploaders" could be easier to understand, instead of only "Num".
config/config.go
Outdated
|
|
||
| // ProxyUploadersConfig stores the configuration for uploaders and queue size in a Proxy backend. | ||
| type ProxyUploadersConfig struct { | ||
| Num int `yaml:"num_uploaders"` |
There was a problem hiding this comment.
I wonder if we should just make these top-level configuration settings since they're identical for each proxy backend? Something like num_proxy_uploaders and max_proxy_queue_size might be reasonable names.
There was a problem hiding this comment.
I thought about that, but the benefit of using a struct is we can check if it's nil and provide the default values (unless the yaml package provides something for that) that have been hard coded. Unfortunately the zero values work against us here and I didn't want users to unwittingly turn off uploads.
Happy to change this though if there's another solution!
There was a problem hiding this comment.
In newFromYaml we pass an empty Config to yaml.Unmarshal, and values are only set in the Config if they exist in the yaml file. Instead of passing an empty Config I think we can pass in one with defaults set.
| } | ||
| }(remote, baseURL, accessLogger, errorLogger) | ||
| } | ||
| errorLogger cache.Logger, numUploaders, maxQueuedUploads int) cache.Proxy { |
There was a problem hiding this comment.
It's very interesting to hear about your local forks. Please do open issues for anything that you think might be worth upstreaming.
I prefer to have individual arguments here- there's less chance of breakage due to changes to raw arguments, compared to a struct.
- Fix existing tests
|
Squashed and landed on the master branch- thanks! |
I'm opening this to see if this satisfies the needs outlined in #351 and gather feedback.
If this looks good I'll fix the tests and write new cases.