initial implementation based on SSE client from Python SDK #1
Conversation
| return self.__will_retry | ||
|
|
||
| @property | ||
| def retry_delay(self) -> float: |
There was a problem hiding this comment.
This is something our other SSE clients don't provide: the ability to see the retry delay programmatically each time it's about to retry. I think this will help in testability: a future version of the contract tests could verify that we're using a backoff based on the number reported here, without having to make unreliable assertions about the actual execution time.
| from typing import Optional | ||
|
|
||
|
|
||
| class RequestParams: |
There was a problem hiding this comment.
I encapsulated the URL + other HTTP options in this type for two reasons: to keep the list of constructor parameters for SSEClient more manageable, and to make it easy for a retry_filter to specify new request parameters if desired.
| The signature of a function that computes a retry delay based on the number of attempts. | ||
|
|
||
| See :func:`ld_eventsource.retry.default_retry_delay_strategy` and | ||
| :func:`ld_eventsource.retry.default_backoff_strategy` for more details. |
There was a problem hiding this comment.
For this and the other "strategy" types, I've used a functional/immutable pattern rather than having the instances contain mutable state. For instance, the default BackoffStrategy maintains a retry counter, but instead of incrementing the retry counter inside itself, it returns an updated clone of itself as part of the BackoffResult. This may be overkill for the fairly simple stuff we're doing here, but I do feel like it helps with testability and it's nice to avoid hidden side effects if possible.
CONTRIBUTING.md
Outdated
|
|
||
| ### Prerequisites | ||
|
|
||
| This project should be developed against its minimum compatible version, Python 3.5. |
There was a problem hiding this comment.
That's a real shame. Can't wait to bump that up in the SDK and then subsequently here.
There was a problem hiding this comment.
Well, 3.5 and 3.6 are now EOL. As I understand it, we still don't want to drop support for them till the next major version of the Python SDK... but, if we don't start using python-eventsource until the next major version of the SDK, then we can drop 3.5 and 3.6 from this project now.
There was a problem hiding this comment.
Are there things from 3.7+ that you think would be particularly helpful in this code?
There was a problem hiding this comment.
No, I wasn't suggesting it because of that. I was more thinking it's a shame to have to support to EOL releases with something completely new, but since it's going to be used in the SDK I totally get that.
I think it is fine to leave it like this for now so you can start using it right away and then we can drop support later.
There was a problem hiding this comment.
...In fact, we should have (or at least could have) already dropped 3.5 in our 7.0.0 release, since it had already been EOL for a month by then. I think that was probably just the result of having started that work a few months earlier.
Co-authored-by: Matthew M. Keeler <keelerm84@gmail.com>
…-eventsource into eb/sc-133323/initial
| version=package_version, | ||
| author='LaunchDarkly', | ||
| author_email='dev@launchdarkly.com', | ||
| packages=['ld_eventsource', 'ld_eventsource.retry'], |
There was a problem hiding this comment.
@keelerm84 I changed this so it no longer uses findpackages— there are only 2 modules I want to export, so this seemed simpler and also findpackages wanted to include testing, which seemed wrong.
This is the first step in creating an independent Python SSE client package, which the Python SDK will use instead of its own current private implementation. The published package name will be
launchdarkly-eventsource.This fully implements all standard SSE behavior(*), and unlike some of our other SSE clients its default behavior is equivalent to the standard EventSource in that it will not auto-retry on HTTP errors. However, it can be easily customized to have the retry behavior that we want in the SDKs, and its backoff/jitter behavior can also be customized (e.g. in the SDKs we want backoffs to reset after a certain interval of non-failure).
(* There is one aspect of the SSE spec where we are more strictly compliant here than in our other SSE clients: we are adding randomized jitter to the computed delay, rather than subtracting it. The spec says that the base delay is a minimum, to which the client can add whatever offset it wants, but in our previous implementations, we treated the base delay as a maximum and subtracted from it. This just means that if we want the same default behavior in the SDK when we switch to using this client, we'll need to multiply the configured base delay by 0.5.)
It also supports several extended behaviors:
One extended behavior it does not yet support is "restart", that is, force-drop an existing connection and retry. I haven't figured out how to make that happen yet, when there is a blocking read in progress; I'm not sure if that's even possible in urllib3 (so I will look into whether we could do it with the lower-level httplib API). That functionality is desirable in order to bring python-server-sdk up to date with the other server-side SDKs' behavior in terms of recovering from database errors (when we will sometimes want to restart the stream), but the current python-server-sdk does not use it.
Both unit tests and contract tests are run in CI, for all supported Python versions. However, we don't yet have contract tests running in Windows.