DM-38578: Improve configuration of HttpResource class #48
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
=======================================
Coverage 85.49% 85.49%
=======================================
Files 27 27
Lines 3633 3731 +98
Branches 746 766 +20
=======================================
+ Hits 3106 3190 +84
- Misses 416 428 +12
- Partials 111 113 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
python/lsst/resources/http.py
Outdated
| class HttpResourcePathConfig: | ||
| """Configuration class to encapsulate the configurable items used by class | ||
| HttpResourcePath. | ||
| """ | ||
|
|
||
| # Default timeouts for all HTTP requests (seconds). | ||
| DEFAULT_TIMEOUT_CONNECT = 30 |
There was a problem hiding this comment.
I assume the drop from 300 to 30 was deliberate?
There was a problem hiding this comment.
Yes, it was deliberate. 300 seconds was too high for a default connection timeout.
python/lsst/resources/http.py
Outdated
| ) | ||
| try: | ||
| self._timeout = ( | ||
| int(os.environ.get("LSST_HTTP_TIMEOUT_CONNECT", self.DEFAULT_TIMEOUT_CONNECT)), |
There was a problem hiding this comment.
You are right. Timeout is a pair of floats. Corrected.
| backoff_factor=30 * (1 + random.random()), | ||
| # (seconds). Compute a random jitter to prevent all the clients | ||
| # to overwhelm the server by sending requests at the same time. | ||
| backoff_factor=self._backoff_min + (self._backoff_max - self._backoff_min) * random.random(), |
There was a problem hiding this comment.
The http client backoff doesn't do this automatically? In the s3 implementation we use the backoff package but I assumed that requests handled that itself.
There was a problem hiding this comment.
The urllib3 implementation of retries (used by the requests package) does not add jitter to the backoff factor passed as argument. I thought it would be more flexible to allow configuring via environment variables an interval from when the backoff is computed including jitter. In the current implementation jitter is added but the backoff factor is kind of fixed to be in the interval [30.0, 60.0) seconds.
| config = HttpResourcePathConfig() | ||
| self.assertFalse(config.collect_memory_usage) | ||
|
|
||
| with unittest.mock.patch.dict(os.environ, {"LSST_HTTP_COLLECT_MEMORY_USAGE": "true"}, clear=True): |
There was a problem hiding this comment.
Another approach would be to take one of the other real tests, patch the environment and then run with assertLogs("DEBUG") and see if you get a memory usage message in the log. It depends on whether you want to test that this flag is set or if this flag has a real effect.
There was a problem hiding this comment.
I considered that the tests of time_this would test that memory usage is actually collected when required. My intention here is to test that you can actually configure that from the environment.
…ntext manager. We only collect memory usage if the LSST_HTTP_COLLECT_MEMORY_USAGE environment variable is set to any value. Systematically collecting memory usage for every operation against the server is too expensive in terms of execution time and is only required for debugging purposes.
* place all the default values inside the class. * add the possibility to configure the collection of memory usage via the environment variable LSST_HTTP_COLLECT_MEMORY_USAGE to avoid expending time collecting this information for every time_this context. * add the possibility to specify the interval for selecting the backoff factor for retrying requests via environment variables LSST_HTTP_BACKOFF_MIN and LSST_HTTP_BACKOFF_MAX.
airnandez
force-pushed
the
tickets/DM-38578
branch
from
6a9db67
to
ee66e1d
Compare
Checklist
doc/changes