Adapters to no longer support delete + set_delete?

[JackCA]

With the updated set of adapters for the 0.5 release, the #set_delete and #delete methods have been removed from the adapter specs. This appeared to be the only way to abstractly delete features/sets from any given adapter and it was definitely nice to have. Is this move completely intentional?

[jnunemaker]

Before I get into my long explanation, can you explain more what you mean by "abstractly delete features/sets". Do you mean you were using the adapters directly instead of the flipper API to enable/disable features?

Explanation

It was very intentional. As I have found, there are basically two ways to make adapters.

1. Make the adapter stupid

The upside is adapters are really easy to make. The downside is they are harder to optimize when it comes to the work involved with checking a given feature. The adapter doesn't know the goal of what flipper is trying to do, so it can't optimize the work to be done based on that goal.

2. Make the adapter smart

The upside is that adapters know what flipper's intent is: get me a feature, enable a feature for a gate/thing, disable a feature for a gate/thing. The downside is that it is a little bit harder to make adapters as adapter authors need to know more about how flipper works and what the intent is. Knowing this though, they can optimize the work to be done, which means more performant adapters and more possibilities.

The reason I changed from option 1 to option 2 is that adapters are used in production far more often than they are written. Ideally, once an adapter is written, it shouldn't need to change unless the client library changes.

Along with that, since the adapter knows flipper's intent, it can fetch all of the related data to a feature at once, instead of piece by piece. As adapters will be used a ton in production code, optimizing for this case is important. I really want there to be minimal impact to use flipper in production.

I hope that makes sense. I have some other ideas for adapter operations, such as preload, which will allow flipper users to preload all the features they need for a request in one network call, instead of several tiny ones. All that will be required is for adapter authors to implement preload in a way that is smart for whatever data store the adapter is wrapping.

Hope this all makes sense. I'm definitely curious to hear what you thoughts are, as you jumped right on the bandwagon. I definitely don't plan on changing this again any time soon, but I wanted to do it now, before even more people join the flipper party.

[jnunemaker]

CC'ing any people that are using flipper in production or might be so they are aware of the adapter changes and why I did it. Also open for any feedback related. It sucks to change formats, but I didn't want to wait until it would be an even bigger nightmare to get people updated.

/cc @chrislloyd @jonduarte @eric

Also, note that each of the adapters now documents how they store things internally, so switching from the old way to the new way should be just a matter of setting a few keys or creating a few documents.

https://github.com/jnunemaker/flipper-redis#internals
https://github.com/jnunemaker/flipper-mongo#internals

Please hit me up with any questions/suggestions regarding these changes.

[JackCA]

@jnunemaker, thank you for the great explanation -- I totally buy into making the adapters smarter.

To answer your first question, no I wasn't using the adapters to directly enable/disable features -- I was using the adapters to delete and add features via add, and delete.

I think an explanation of why I was doing this is in order:

It basically comes down to syncing features on startup -- while our code can make reference to $flipper[:foo].enabled? all it wants, unless foo has been explicitly added to the feature set at any point (or disabled/enabled), it will not show up in the features set. This is important because we have a UI interface (something I know you have in the works), and we need to show exactly what features are available with the current codeset. We don't want to show features that are no longer behind the flipper, and we don't want to have to add new features by hand on deploys.

The solution: our flipper initializer parses a manifest.yml file which contains an array of features by name and adds missing features and removes any that are no longer in use, keeping everything in sync.

Now, the adding portion is still fine, but the absence of delete and set_delete hurt because our development, test, and production could potentially all be using different adapters which would mean 3 different implementations of delete in our own codebase.

I know this may be a unique use-case for the delete methods, but I would think that once this gets out there, this sort of work flow would be common.

Essentially the question is, "how do I delete a feature now?"

[jnunemaker]

Gotcha. I could certainly add a remove method that is the opposite of add. It would remove the feature from the set of known features and also clear all gate values for the feature. Seem like a plan?

[JackCA]

That sounds perfect. I know it's more overhead for implementing new adapters, but it will be definitely be a helpful method moving forward. Let me know if I can be of any help

[jnunemaker]

@JackCA #21 look good to you?

[JackCA]

Perfect! Way to go 👍 Very exciting 👏