Add polling + indirect support for streaming #47

drichelson · 2016-06-30T01:19:05Z

I tried to change as little as possible, but that didn't work. See comments in diff.
This is deployed to restwrappers and harness is running: https://circleci.com/gh/launchdarkly/integration-harness/1126

…lling and streaming restwrappers pass, but local tests do not pass.

…lling and streaming restwrappers pass, All but some integration tests pass

jkodumal · 2016-06-30T01:22:05Z

ldclient/client.py

        self._session = CacheControl(requests.Session())
        self._queue = queue.Queue(self._config.capacity)
        self._consumer = None
        self._offline = False


One thing we did in the other refactorings like this is change offline mode to a configuration parameter rather than a modifiable state. This makes it a lot easier to reason about things, and it makes it the case that we don't have to lazily initialize the streaming connection.

drichelson · 2016-06-30T01:37:31Z

ldclient/twisted_redis.py



-class TwistedRedisLDDStreamProcessor(StreamProcessor):
+class TwistedRedisLDDStreamProcessor(UpdateProcessor):


Do we need this? It seems like this should be a RedisFeatureStore, not a Redis stream processor..

Indeed, I think this should be refactored into a RedisFeatureStore.

This would also involve substantial refactoring to RedisLDDRequester, but I think we should bring everything in line with the structure of the other implementations anyway.

…ly read after that.

drichelson · 2016-06-30T16:26:00Z

This is passing against restwrappers running locally, but is failing on Circle. I'm investigating, but I believe all this code to be correct.

jkodumal · 2016-06-30T16:58:08Z

ldclient/client.py

-
 class LDClient(object):
-
    def __init__(self, api_key, config=None):


Can we add an option (as we have in go) to wait for initialization? That would get rid of the ugly sleep in thet demo code.

jkodumal · 2016-06-30T18:52:45Z

I would recommend updating the major version in version.py now as part of this refactoring.

jkodumal · 2016-06-30T19:01:07Z

ldclient/polling.py

+        self._config = config
+        self._requester = requester
+        self._store = store
+        self._running = False


I don't think there are any implications to this, and I notice we use the same pattern in the StreamProcessor, but this doesn't look thread safe to me.

Do you mean that the modification of self._running is not done under a lock?

…disable waiting for startup.

drichelson · 2016-07-06T20:35:15Z

3 existing restwrappers: python with streaming, python polling, and twisted streaming:
https://circleci.com/gh/launchdarkly/integration-harness/1173 (python)
https://circleci.com/gh/launchdarkly/integration-harness/1174 (twisted)

drichelson · 2016-07-06T20:48:28Z

ldclient/__init__.py

+_lock = threading.Lock()
+
+
+def get():


This follows the model used by segment to enforce the singleton pattern. See updated readme + restwrapper PR: https://github.com/launchdarkly/python-restwrapper/pull/1/files

drichelson · 2016-07-06T21:30:34Z

Added python-redis restwrapper, it should run with this build: https://circleci.com/gh/launchdarkly/integration-harness/1176

drichelson · 2016-07-06T23:10:07Z

The megabuild: https://circleci.com/gh/launchdarkly/integration-harness/1176 failed the polling restwrapper, but I ran it again: https://circleci.com/gh/launchdarkly/integration-harness/1177 and it passed. Then I ran both python wrappers: https://circleci.com/gh/launchdarkly/integration-harness/1178 and they both passed..

pkaeding · 2016-07-07T16:29:29Z

demo/demo.py

-    apiKey = 'feefifofum'
-    client = LDClient(apiKey)
+    api_key = 'api_key'
+    client = LDClient(api_key, start_wait=10)


Should this be calling ldclient.get() like the README says?

pkaeding · 2016-07-07T16:45:41Z

As there are a number of breaking changes here (I noticed the major version bump, so that is good), we should be sure to call out those changes in the release notes.

(remember that files are modules in Python, so moving a class to its own file is moving it to a new module, which is a breaking change)

pkaeding · 2016-07-07T17:18:27Z

ldclient/redis_feature_store.py

+                 url='redis://localhost:6379/0',
+                 prefix='launchdarkly',
+                 max_connections=16,
+                 expiration=15,


these parameters should probably be documented

pkaeding · 2016-07-07T17:34:40Z

lgtm
👍
looks good to me
merge away
I approve
canned replies are great.

jkodumal · 2016-07-07T18:32:02Z

Let's have this PR target a v2 branch, instead of master.

drichelson · 2016-07-07T18:42:54Z

ldclient/__init__.py

+_lock = ReadWriteLock()
+
+
+def get():


Added double-check locking. Please run through how this can break.

This looks reasonable to me.

drichelson · 2016-07-07T20:03:43Z

ok- I'm closing this PR and merging this branch into v2.

drop Python 2.6 support

Dan Richelson added 5 commits June 28, 2016 13:55

[wip] Added polling support + indirect messages in streaming. Both po…

280d1c4

…lling and streaming restwrappers pass, but local tests do not pass.

[wip] Added polling support + indirect messages in streaming. Both po…

02b7f2d

…lling and streaming restwrappers pass, All but some integration tests pass

cleanup

c4501cd

polling client no longer drifts.

ce05217

cleanup

2b74410

jkodumal reviewed Jun 30, 2016
View reviewed changes

Remove commented code. Fix some documentation

22382ed

drichelson reviewed Jun 30, 2016
View reviewed changes

Move offline to config so it is set once during initialization and on…

9e9fe54

…ly read after that.

drichelson changed the title ~~Add polling + indirect support for streaming~~ [HOLD] Add polling + indirect support for streaming Jun 30, 2016

drichelson changed the title ~~[HOLD] Add polling + indirect support for streaming~~ Add polling + indirect support for streaming Jun 30, 2016

Add log statements when returning default

222f981

jkodumal reviewed Jun 30, 2016
View reviewed changes

Dan Richelson added 3 commits June 30, 2016 18:31

Rename

ea76873

Rename and move some things.

f29e2a0

Rename some config params. Add Redis feature store + tests

c34a29f

Dan Richelson added 3 commits July 3, 2016 20:36

Revert some things that didn't work.

23547c1

Attempt to make event consumer behavior more consistent. temporarily …

7110730

…disable waiting for startup.

Bump version. Rename some things.

4c819c5

drichelson changed the title ~~Add polling + indirect support for streaming~~ [HOLD] Add polling + indirect support for streaming Jul 5, 2016

Dan Richelson added 6 commits July 6, 2016 11:48

Add start_wait.

ab4929c

add future

4e32f00

cahnge log statement.

62ceb43

Make python 3 happy.

739ccb1

Make python 3 happy.

0596d50

Start polling processor for real this time

1350833

drichelson changed the title ~~[HOLD] Add polling + indirect support for streaming~~ Add polling + indirect support for streaming Jul 6, 2016

Update readme

f2cb7a3

drichelson reviewed Jul 6, 2016
View reviewed changes

Dan Richelson added 2 commits July 6, 2016 14:15

Change redis feature store to take url

9c23fc4

always set a feature store

467ad4e

pkaeding reviewed Jul 7, 2016
View reviewed changes

Address PR comments. Fix weird formatting in test file.

605fb13

drichelson reviewed Jul 7, 2016
View reviewed changes

drichelson closed this Jul 7, 2016

eli-darkly added a commit that referenced this pull request May 10, 2018

Merge pull request #47 from launchdarkly/eb/ch15531/drop-python-2.6

108bf10

drop Python 2.6 support



		class TwistedRedisLDDStreamProcessor(StreamProcessor):
		class TwistedRedisLDDStreamProcessor(UpdateProcessor):


		class LDClient(object):

		def __init__(self, api_key, config=None):

Add polling + indirect support for streaming #47

Add polling + indirect support for streaming #47

Uh oh!

Conversation

drichelson commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkodumal Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drichelson commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkodumal commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkodumal Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drichelson commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drichelson commented Jul 6, 2016

Uh oh!

drichelson commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkaeding commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkaeding commented Jul 7, 2016

Uh oh!

jkodumal commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drichelson commented Jul 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drichelson commented Jun 30, 2016 •

edited

Loading

jkodumal Jun 30, 2016 •

edited

Loading

jkodumal commented Jun 30, 2016 •

edited

Loading

jkodumal Jun 30, 2016 •

edited

Loading

drichelson commented Jul 6, 2016 •

edited

Loading

pkaeding commented Jul 7, 2016 •

edited

Loading