Allow SDK key/config to be changed after client has been initialized. by drichelson · Pull Request #64 · launchdarkly/python-server-sdk

drichelson · 2016-11-05T21:49:52Z

The reasoning behind this is that it makes the reset SDK Key feature much more managable when a dynamic configuration is in place.

The intention is that this is an api-breaking change for anyone using the new singleton flow found in init.py, but anyone still using the client constructor should not have to make any code changes.

drichelson · 2016-11-07T17:33:30Z



-def get():
+def get(sdk_key=None):


This is the least terrible pattern I could think of: first call get(sdk_key), then on subsequent calls it is optional..
I still don't like it though

drichelson · 2016-11-08T02:26:04Z

[ch138]

drichelson · 2016-11-11T02:04:18Z

-    ldclient.sdk_key = 'sdk_key'
    ldclient.start_wait = 10
-    client = ldclient.get()
+    ldclient.set_sdk_key('YOUR_SDK_KEY')


Example of new usage pattern.

drichelson · 2016-11-11T02:04:49Z

+# 2 use cases:
+# 1. Initial setup: sets the sdk key for the uninitialized client
+# 2. Allows on-the-fly changing of the sdk key. When this function is called after the client has been initialized
+#    the client will get re-initialized with the new sdk key. In order for this to work, the return value of


This is the pitfall of this approach, but I don't see another way to do it. any ideas @pkaeding ?

I don't think we should say that it should never be assigned; that seems a bit extreme. It is fine to get the client at the beginning of a request handler that needs to get a bunch of flags. You just don't want to keep the reference long-term.

On the other hand, we believe the population of users who need to change the SDK key at runtime is rather small. So most people can disregard this warning altogether if they like. I think we can re-word this to be less scary, while still explaining enough of the implications of different use cases.

I'm almost inclined to avoid this wording, except when documenting how to change the config in an already-running app.

drichelson · 2016-11-11T02:06:01Z

 from cachecontrol import CacheControl
 from threading import Lock

-GET_LATEST_FEATURES_PATH = '/sdk/latest-flags'


config code now in its own file.

In order to make this change backwards compatible, I think you could alias the Config class in this package

drichelson · 2016-11-11T02:06:33Z

@@ -0,0 +1,171 @@
+from ldclient.event_consumer import EventConsumerImpl


The refactored Config class is as immutable as python allows..

pkaeding · 2016-11-11T06:18:04Z

+    global __config
+    global __client
+    global __lock
+    if sdk_key is __config.sdk_key:


this should probably be guarded by the read lock, right?

pkaeding · 2016-11-11T06:19:40Z

-        global client
-        _lock.lock()
-        if not client:
+        global __client


I don't think this is needed, since you declared global __client above.

pkaeding · 2016-11-11T06:22:45Z

+
+
+# the return value should not be assigned.
 def get():


In the README, you give an example of:

ldclient.get("your sdk key")

Should this be:

def get(sdk_key=None):

apologies- readme is not up to date.

pkaeding · 2016-11-11T06:30:08Z


 class LDClient(object):
-    def __init__(self, sdk_key, config=None, start_wait=5):
+    def __init__(self, config=None, start_wait=5):


To try to make this backwards-compatible, I think you can also allow the sdk_key to be passed to this ctor, and apply it to the config if it is passed in.

If there is some reason not to do that (that I'm missing), then the config argument should not be optional-- it is not valid to create a client with no SDK key.

it is valid to create an offline client with no sdk key, or a client with a redis feature store (events of course will fail or can be disabled)

Also, directly creating a client is no longer the suggested route, so I prefer to have it only take in the config object.

Ahh, good point about the offline client. I get that we don't want to encourage people to directly create the client, but it might ease the transition if they could drop in the new version, and have everything work as it did before, modulo some deprecation warnings that they can then work to eliminate.

pkaeding · 2016-11-11T06:33:25Z


 class EventConsumerImpl(Thread, EventConsumer):
-    def __init__(self, event_queue, sdk_key, config):
+    def __init__(self, event_queue, config):


This (and below) is a little harder to rationalize the backwards-compatible way of overlaying an optional sdk_key argument on the config.

Maybe doing that, and marking it deprecated would be the way to go? Then we can remove it in the next major release.

These changes necesitate a major release (due to changes in singleton init flow). Does this change your take on this?

pkaeding · 2016-11-11T06:42:35Z

I think this approach makes sense. I would like to see if we can possibly do it in a backwards-compatible way, to avoid needing a major release, and make upgrading easier for our users. I'm not totally sure if that is possible, though.

drichelson · 2016-11-11T17:50:08Z

+        __lock.unlock()
+
+
+def init():


My thinking here is that this could be called in server init code to pre-warm the client so the first request doesn't incur the penalty of connecting to the stream etc... on second thought I have removed it. Thoughts?

…th previous versions.

pkaeding · 2016-11-11T19:18:45Z

        check_uwsgi()
-        self._config = config or Config.default()
+
+        if config is not None and sdk_key is not None:


I think this should check if the skd_key is set in both places, not if any config is passed in. Some existing clients might be using a modified config (changed timeouts, etc), but passing in the sdk_key separately.

good point- this could break existing patterns..

…th previous versions.

pkaeding · 2016-11-11T19:48:01Z

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

jkodumal · 2016-11-17T19:53:28Z

👍

Add Python 3.7-rc to build

Dan Richelson added 3 commits November 4, 2016 15:19

Api-breaking changes. Allow SDK key to be changed.

95bcba8

fix test import

620b394

remove property decorator

68d41c8

drichelson commented Nov 7, 2016

View reviewed changes

drichelson changed the title ~~Allow SDK key to be changed after client has been initialized.~~ [HOLD] Allow SDK key to be changed after client has been initialized. Nov 8, 2016

Dan Richelson added 3 commits November 10, 2016 10:33

Move config class to its own file

b855c28

[wip] things work, but not ideally.

f224804

[wip] Allow changing of config.

b4f8036

drichelson commented Nov 11, 2016

View reviewed changes

drichelson changed the title ~~[HOLD] Allow SDK key to be changed after client has been initialized.~~ Allow SDK key to be changed after client has been initialized. Nov 11, 2016

drichelson changed the title ~~Allow SDK key to be changed after client has been initialized.~~ Allow SDK key/config to be changed after client has been initialized. Nov 11, 2016

pkaeding reviewed Nov 11, 2016

View reviewed changes

drichelson commented Nov 11, 2016

View reviewed changes

Address PR comments. Fix tests. Make client constructor compatible wi…

dcb85df

…th previous versions.

pkaeding reviewed Nov 11, 2016

View reviewed changes

Address PR comments. Fix tests. Make client constructor compatible wi…

259eaf0

…th previous versions.

Dan Richelson added 4 commits November 14, 2016 11:45

Set sdk key when client has not been initialized.

2ef65ba

increase test timeout

68cdba3

[temp] change circle file to build python3 first.

4d2e97f

Revert circle.yml change

b415f1e

Dan Richelson added 3 commits November 14, 2016 14:35

Update sse client to fix large initial payload.

917315b

Bring in sse client and fix it for python3

2c44e5f

update dependency versions

8d7c532

pkaeding approved these changes Nov 15, 2016

View reviewed changes

Lower future version for compatibility

62ecd42

drichelson merged commit 2125c5a into master Nov 17, 2016

drichelson deleted the dr/changeSdkKey branch November 17, 2016 20:04

eli-darkly added a commit that referenced this pull request Aug 3, 2018

Merge pull request #64 from launchdarkly/eb/ch19764/py3.7

515aa3c

Add Python 3.7-rc to build

		@@ -0,0 +1,171 @@
		from ldclient.event_consumer import EventConsumerImpl

Conversation

drichelson commented Nov 5, 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!

drichelson commented Nov 8, 2016

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

pkaeding Nov 11, 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!

pkaeding commented Nov 11, 2016

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!

pkaeding commented Nov 11, 2016

Uh oh!

jkodumal commented Nov 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

drichelson commented Nov 5, 2016 •

edited

Loading

pkaeding Nov 11, 2016 •

edited

Loading