Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Drop async support #183

Closed
flovilmart opened this issue Apr 8, 2020 · 4 comments
Closed

Drop async support #183

flovilmart opened this issue Apr 8, 2020 · 4 comments

Comments

@flovilmart
Copy link

flovilmart commented Apr 8, 2020

Is your feature request related to a problem? Please describe.

The node SDK comes with batteries included but also installs a bunch of packages that may never be used (like redis or winston).
Redis also forces the design of the SDK to be also fully which in turn creates a bunch of promises and closures. I also forces the consumer to switch to async consumtion of the features while all the calls underneath are truly synchronous.

Describe the solution you'd like

The feature store can be rewritten to be synchronous, I do not really understand the benefit of using redis as an intermediary store vs in memory as the LD server keep pushing data to the SDKs. I believe that if there is such a requirement, it could be possible to keep the feature store synchonous (a local copy) an poll redis for the config.

Sample implementation

master...flovilmart:sync-sdk

This would benefit the vast majority of LD users. I'm eager to get some feedback around the need of using redis as a store.

@eli-darkly
Copy link
Contributor

There are two parts to this issue.

  1. The use of asynchronous semantics in the Node SDK. This has been discussed at length, most recently here: VariationDetails sync method #177

Short version: in order for the SDK to ever support any persistent data store mechanism, it needs to be async. We could, as you suggest, write a synchronous version, but it would need to be an almost total rewrite; only the analytics event system would be reusable. And due to the difference in the calling semantics, that would make it a big pain for someone to prototype an application using an in-memory data store and then switch to a persistent store if their deployment strategy changes; it's less than ideal for the entire mode of using the SDK to change like that just because one feature is being used differently.

  1. "I do not really understand the benefit of using Redis as an intermediary store." This is not a Node SDK issue, it has to do with the entire product design and it's pretty far outside the scope of this repo for me to get into it here, but the reason we offer this feature is that many other customers do find it useful (I don't have statistics, but I'm not sure your assumption that "the vast majority" will never use a database is correct). It has been a core feature since the early days of our product.

The two main use cases are: a) If your app has to restart and finds that it's unable to connect to LD at that point, it still has a backup of last known data. b) It can be used in an alternate deployment model where, instead of app instances connecting to LD, a Relay Proxy instance connects to LD and provides flags to the app via the database. These scenarios are discussed in some detail in the online docs here and here.

I'm closing this issue because the previous issue covers the same ground, but to be clear, we're not entirely ruling out doing something different with this in the future; we're aware that async APIs have overhead and can be a hassle. The idea of polling the database is one option we've been considering, although that brings up a bunch of other design issues - I won't get into that here either, I'll just say that it is not as straightforward as it might sound to do that efficiently.

@flovilmart
Copy link
Author

but it would need to be an almost total rewrite

You can check the code, it's re-written already.

That's a bummer, but we'll keep running our fork.

Thanks!

@eli-darkly
Copy link
Contributor

@flovilmart I have seen the code. The changes are straightforward; I wasn't complaining that we would need to figure out how to do them, it's really just equivalent to porting the same logic that we use now in other languages. The issue is that we would not be able to share any of the flag evaluation logic between the regular SDK and this new version, so we would really be maintaining two almost completely separate SDKs. I don't mean to seem ungrateful for your efforts, but the reason we haven't done this already was not because it was unclear how to do it.

@flovilmart
Copy link
Author

I believe there’s still a large part of the evaluation logic that could be used. But I understand, it’s a totally different paradigm and I also understand the added complexity and burden. I didn’t mean to be pushy or anything of the sort.

LaunchDarklyCI pushed a commit that referenced this issue Jun 30, 2020
…steners

reuse same Promise and same event listeners for all waitForInitialization calls
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants